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

Fixed issue with modifying WordLists #235

Merged
merged 5 commits into from Nov 21, 2018

Conversation

@jammmo
Copy link
Contributor

commented Nov 10, 2018

Fixed an issue where WordLists effectively stored two different lists (one as the instance variable self._collection, and one as the WordList itself, since it inherits the list class). This caused a variety of unexpected behavior, such as the pop() method appearing not to modify the appearance of WordList, because it only modified the inherited list while the __str__ and __repr__ methods referenced the instance variable. Resolved by eliminating self._collection and modifying several methods to use the inherited list instead.

jammmo added some commits Nov 10, 2018

Fixed issue with modifying WordLists
Fixed an issue where WordLists effectively stored two different lists (one as the instance variable self._collection, and one as the WordList itself, since it inherits the list class). This caused a variety of unexpected behavior, such as the pop method appearing not to modify the appearance of WordList, because it only modified the inherited list while the __str__ and __repr__ methods referenced the instance variable. Resolved by eliminating self._collections and modifying several methods to use the inherited list instead
Corrected WordList string representation
Fixed __str__ method to match existing convention (prints without the class name, as opposed to __repr__ which uses the class name)
@sloria
Copy link
Owner

left a comment

Thanks @jammmo for the PR!

Can you please add a regression test for this change?

jammmo added some commits Nov 15, 2018

Added regression tests
Added WordList tests for pop, __setitem__, and reverse, to ensure that these methods behave as they would for an ordinary Python list
@jammmo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

Sure thing. I added tests for pop, __setitem__, and reverse, which were the methods I originally noticed this problem with. It might also be a good idea to add a test for equality, since this resolved an issue there too (i.e. if you had two identical WordLists and appended to one of them, it said they were equal since the __eq__ method was inherited but append didn't use the inherited list). I wasn't entirely sure what the correct behavior for __eq__ should be, though. Should it consider POS tags or just compare each word in the list like a string?

@jammmo

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

@sloria ?
(My bad, forgot to tag you earlier)

@sloria

sloria approved these changes Nov 21, 2018

@sloria sloria merged commit 9ac110a into sloria:dev Nov 21, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sloria

This comment has been minimized.

Copy link
Owner

commented Nov 21, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.