Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented .pop() method for list #126

Open
wants to merge 1 commit into
base: master
from

Conversation

@xarus01
Copy link
Contributor

xarus01 commented Nov 6, 2019

Fixes #95.

py/list.go Outdated Show resolved Hide resolved
@xarus01 xarus01 force-pushed the xarus01:implement_list_pop_i95 branch from 2c4399a to 84a1729 Nov 18, 2019
@xarus01 xarus01 force-pushed the xarus01:implement_list_pop_i95 branch from 84a1729 to 0966919 Nov 18, 2019
@sbinet
sbinet approved these changes Nov 18, 2019
@corona10
Copy link
Member

corona10 commented Nov 18, 2019

@xarus01 CI is failed. Please take a look

@ncw
Copy link
Collaborator

ncw commented Nov 18, 2019

Here is the failure. This is trying to run the tests under python3.4.

============================================================
Failures for /usr/bin/python3.4 in ./py/tests/list.py
============================================================
Traceback (most recent call last):
  File "./py/tests/list.py", line 40, in <module>
    assert a.pop() == 1
AssertionError
Original exception was:
Traceback (most recent call last):
  File "./py/tests/list.py", line 40, in <module>
    assert a.pop() == 1
AssertionError
============================================================

(there was more stuff I've snipped out the error which seems to be python trying to report errors via apport and failing 😕 )

assert a.pop() == 1
assert a.pop(2) == 4
Comment on lines +40 to +41

This comment has been minimized.

@tim-st

tim-st Dec 1, 2019

Contributor

The pop operation removes and returns from the back, I think your idea was removing from the front.
It would be good if you create a new list for each test and compare your test against a list like assert a.pop(-1) == element; assert a = [...], that makes it more clear here.

Additionally the following tests would be good:

  • negative index
  • index out of range for non-empty list
  • parameter of wrong type (non-int)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.