Skip to content

Implemented .pop() method for list #126

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Implemented .pop() method for list #126

wants to merge 1 commit into from

Conversation

xarus01
Copy link
Contributor

@xarus01 xarus01 commented Nov 6, 2019 •
edited by sbinet
Loading

Fixes #95.

sbinet
sbinet reviewed Nov 6, 2019
View reviewed changes
@xarus01 xarus01 force-pushed the implement_list_pop_i95 branch from 2c4399a to 84a1729 Compare November 18, 2019 08:17
@xarus01 xarus01 force-pushed the implement_list_pop_i95 branch from 84a1729 to 0966919 Compare November 18, 2019 08:18
@corona10
Copy link
Collaborator

@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 😕 )

ghost
ghost reviewed Dec 1, 2019
View reviewed changes
py/tests/list.py
Comment on lines +40 to +41
assert a.pop() == 1
assert a.pop(2) == 4
Copy link

@ghost ghost Dec 1, 2019 •
edited by ghost
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@xarus01 xarus01 closed this Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbinet sbinet sbinet approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement list pop method
4 participants
@xarus01 @corona10 @ncw @sbinet