dibs: implement simple-linked-list by laurauzcategui · Pull Request #385 · exercism/python · GitHub

Opened also: #384

@kytrinyx could you please review? I'm new to exercism and I was trying to figure it out if this was the right way to get started.

linked-list is a quite similar exercise #212 which might help you while working on this new exercise. There you can see how to add the exercise to the config.json.
For possible test cases you could have a look at other tracks like C# or ruby. We use flake8 as linter (details). You might need to run that, to fix some of your code style problems.

Thanks @behrtam 👍 Fixed the issues with flake8. Will be adding test cases and other helpers with progress of the days.

Cheers,

@behrtam will you have time to review (and potentially merge) before the end of Hacktoberfest (Monday, end of day)?

@laurauzcategui there's a conflict in the config.json—would you mind fixing it?

Pull requests do not have to be merged and accepted; as long as they've been opened between the very start of October 1 and the very end of October 31, they count towards a free T-shirt. – source

So, @kytrinyx there is no rush to merge this today and I will review it probably this week.

@laurauzcategui The conflict with the config.json can be resolved with a rebase of upstream/master.

Hi @behrtam, @kytrinx

Thanks. Simple-linked-list is not ready yet, but binary-search-tree I believe it is :-)

And as @behrtam said, there is no rush for Hacktoberfest :) I already received the email from digitalOcean that my PRs are valid :)

Hi @behrtam , @kytrinyx , now it's good to go for a review.

Happy 🎃

Cheers,

Beside some details my first impression is good but we do have a more abstract problem. Normally singly linked lists insert and delete at the beginning of the list therefor the complexity is O(1). This is also the behaviour mentioned in the readme of the problem "push-down stacks (also called a LIFO stack; Last In, First Out)".
Sadly we don't have a canonical-data.json for this problem, that would make it more clear. As a result while the ruby track tests exactly for this (test_items_are_stacked), the C# track does it the other way around (Two_item_list_second_value).

Until we have canonical test data and we found a way to harmonize all the tracks, I would go with the readme and change your behaviour. push should add to the head, pop should take from the head, peek should give you the value of the head.

Hi @behrtam updated based on your recommendations. Looking for a new review :)

Cheers,

Great! I should find some time to review that in the next days.

This issue has been automatically marked as `on hiatus because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@behrtam Did this one fall between the cracks? Would you have time to give it a look?

Oh, I'm sorry. Should finde some time on Sunday.

Awesome, thanks! 🌻

Okay, finally I found some time ...

We have decided to use stub files for all exercises (#415) so you would need to add this here as well.

Right now you have quite complex tests, one test case for each functionality. Instead I would like to see tests that are more concise (in most cases one assert per test) which work in small incremental steps. Maybe the ruby tests can be of some help: https://github.com/exercism/xruby/blob/master/exercises/simple-linked-list/simple_linked_list_test.rb

You might have to rebase to update this PR with some of the changes that happened in the meantime.

The naming needs to be improved. Right now one can push values, than the parameter should be named accordingly or the method should accept Elements instead.

The convention is that reverse/sort/etc work change the actual instance, while reversed/sorted/etc would return a new changed instance leaving the other instance unchanged.

You don't need to check size and the current element, one check is sufficient. It makes the code more understandable:

def reversed(self):
    new_list = LinkedList()
    current = self.head
    while current:
        new_list.push(current.value)
        current = current.next
    return new_list

The head argument is never used. Maybe it could be used for the from array list creation instead.

I would either implement the from_array initializer using the __init__(self, array=None) method or a @classmethod (see: https://stackoverflow.com/a/682545).

This method also doesn't need two checks for the same thing. There is also no need in cutting variable names short.

def to_array(self):
    array = []
    current = self.head
    while current:
        array.append(current.value)
        current = current.next
    return array

No need for undescriptive short variable names, there is enough space for element = self....

We have decided to use stub files for all exercises (#415) so you would need to add this here as well.

@behrtam I'd recommend documenting this in the README.

This issue has been automatically marked as on hiatus because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Has this been implemented or just closed due to inactivity?

This is closed for inactivity. It doesn't look like anyone else has implemented in the mean time (it would show up in this list on master: https://github.com/exercism/python/tree/master/exercises)

@behrtam reviewed it and asked for some changes. If you want to carry the changes and implement the fixes that @behrtam suggested that would be awesome.

Check out Jess Frazelle's blog post, The Art of Closing, especially point 4 "The Carry" if you're interested in helping out with this. https://blog.jessfraz.com/post/the-art-of-closing/

I'd like to try implementing these changes, I'm somewhat of a newbie to git and exercism. I've done a few python exercism exercises on my own but I want to contribute to this one with alittle help. The first question is how do I go about seeing the files that need changes? Second, then after making any edits, how do I recommit them? Is it the process from this link? https://github.com/exercism/docs/blob/master/you-can-help/implement-an-exercise-from-specification.md

Step 1) Fork this repository and clone it locally. If you don't know how to do this take a look at this guide: https://help.github.com/articles/fork-a-repo/

Step 2) Add the upstream repo as remote. Details on how to do this are here: https://help.github.com/articles/configuring-a-remote-for-a-fork/

Step 3) Fetch the PR into a local branch, to continue the work and later create a new PR. The steps on how to checkout a PR locally can be found here: https://help.github.com/articles/checking-out-pull-requests-locally/
Keep in mind that in this case you don't want to fetch from origin but from upstream.

So, for me this looks like this:

➜  python git:(master) git remote -v
origin	git@github.com:behrtam/python.git (fetch)
origin	git@github.com:behrtam/python.git (push)
upstream	https://github.com/exercism/python.git (fetch)
upstream	https://github.com/exercism/python.git (push)

➜  python git:(master) git fetch upstream pull/385/head:carry-on-pr-385
remote: Counting objects: 80, done.
remote: Total 80 (delta 21), reused 21 (delta 21), pack-reused 59
Unpacking objects: 100% (80/80), done.
From https://github.com/exercism/python
 * [new ref]         refs/pull/385/head -> carry-on-pr-385

➜  python git:(master) git checkout carry-on-pr-385
Switched to branch 'carry-on-pr-385'

➜  python git:(carry-on-pr-385) python3 test/check-exercises.py simple-linked-list
.....
----------------------------------------------------------------------
Ran 5 tests in 0.000s

OK

➜  python git:(carry-on-pr-385)

If you have any questions or need any help. Just ask.

Thanks for information, I did steps 1 and 2. However, the checkout didn't work. The images below said local changes would be overwritten by checkout.

Any thoughts?

Other questions I have is I'm not entirely sure how to setup testing for this exercise. I looked at these links below and want to know if I'm in the right direction.

https://github.com/exercism/exercism.io/blob/master/CONTRIBUTING.md
https://github.com/exercism/exercism.io/blob/master/docs/setting-up-local-development.md

laurauzcategui added 2 commits October 26, 2016 22:42

dibs: I will implement exercise simple-linked-list

58efa25

dibs: implemet simple-linked-list

927f136

Adding data structures pop peak empty methods and test class

29800e5

LauraU added 2 commits October 27, 2016 11:01

simple-linked-list: fixing flake8 issues

bd03ee7

including slug simple-linked-list in config.json

cfd8524

Adding data structures pop peak empty methods and test class

52d24df

laurauzcategui added 3 commits October 31, 2016 14:24

including test cases for simple-linked-list

6c3ea1e

merging with conflicting files, fixing config.json

7f2e8ca

Fixing flake8 issues on test file

795a81b

laurauzcategui and others added 10 commits November 19, 2016 16:44

Adding data structures pop peak empty methods and test class

14a219c

simple-linked-list: fixing flake8 issues

4e81131

including slug simple-linked-list in config.json

e2f6b83

Adding data structures pop peak empty methods and test class

9ca4ff2

including test cases for simple-linked-list

71687b7

Fixing flake8 issues on test file

ed992fc

Updating unit test and behaviour of pop and push to be O(1)

44b2bf9

Merging from branch simple-linked-list

8c031c0

fixing flake8 issues and merge

6bef500

Fixing flake8 issues in Python3

93321b9

behrtam mentioned this pull request Feb 22, 2017

Should we forgo any missing exercises #417

Closed

stale Bot added the on hiatus label May 6, 2017

stale Bot removed the on hiatus label May 19, 2017

behrtam reviewed May 28, 2017

View reviewed changes

stale Bot added the on hiatus label Jul 27, 2017

stale Bot closed this Aug 3, 2017

eliaahadi reviewed Sep 10, 2017

View reviewed changes

ilya-khadykin mentioned this pull request Oct 6, 2017

simple-linked-list: implement exercise #738

Closed

ilya-khadykin added the abandoned label Oct 8, 2017

+                      if self.empty():
+                          return None
+                      else:
+                          e = self.head

+                      self.head = head
+                      self.size = 0
+                  def push(self, new_element):

+                      else:
+                          return self.head.value
+                  def reverse(self):

+                      arr.append(current.value)
+                      return arr
+                  def from_array(self, arr=[]):

dibs: implement simple-linked-list by laurauzcategui · Pull Request #385 · exercism/python · GitHub

Repository navigation

dibs: implement simple-linked-list - #385

dibs: implement simple-linked-list#385laurauzcategui wants to merge 19 commits intoexercism:masterexercism/python:masterfrom laurauzcategui:simple-linked-listlaurauzcategui/xpython:simple-linked-listCopy head branch name to clipboard

Conversation

laurauzcategui commented Oct 26, 2016

Uh oh!

laurauzcategui commented Oct 26, 2016

Uh oh!

behrtam commented Oct 27, 2016

Uh oh!

laurauzcategui commented Oct 27, 2016

Uh oh!

kytrinyx commented Oct 30, 2016

Uh oh!

kytrinyx commented Oct 30, 2016

Uh oh!

behrtam commented Oct 31, 2016

Uh oh!

laurauzcategui commented Oct 31, 2016

Uh oh!

laurauzcategui commented Oct 31, 2016

Uh oh!

behrtam commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laurauzcategui commented Nov 19, 2016

Uh oh!

behrtam commented Nov 22, 2016

Uh oh!

stale Bot commented May 6, 2017

Uh oh!

kytrinyx commented May 19, 2017

Uh oh!

behrtam commented May 19, 2017

Uh oh!

kytrinyx commented May 19, 2017

Uh oh!

behrtam left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

behrtam May 28, 2017

Choose a reason for hiding this comment

Uh oh!

kytrinyx commented May 28, 2017

Uh oh!

stale Bot commented Jul 27, 2017

Uh oh!

eliaahadi commented Sep 8, 2017

Uh oh!

kytrinyx commented Sep 8, 2017

Uh oh!

eliaahadi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behrtam commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eliaahadi commented Oct 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dibs: implement simple-linked-list#385
laurauzcategui wants to merge 19 commits into
exercism:masterfrom
laurauzcategui:simple-linked-list

behrtam commented Nov 1, 2016 •
edited
Loading

behrtam left a comment •
edited
Loading

eliaahadi left a comment •
edited
Loading

behrtam commented Sep 12, 2017 •
edited
Loading

eliaahadi commented Oct 3, 2017 •
edited
Loading