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

Several issues noticed while skimming the code... #1

Closed
njsmith opened this issue Jun 16, 2017 · 2 comments
Closed

Several issues noticed while skimming the code... #1

njsmith opened this issue Jun 16, 2017 · 2 comments

Comments

@njsmith
Copy link

njsmith commented Jun 16, 2017

  • Multiset.__delitem__ is untested and appears to be broken: it doesn't update self._total. Maybe it should be implemented as self[element] = 0 to reuse the __setitem__ code?

  • The docstring for BaseMultiset.union says "For a variant of the operation which modifies the multiset in place see :meth:`union`.". But... that's what I'm looking at :-)

  • Your package metadata says you support both py2 and py3, but the wheel on pypi is marked as being py3-only. Maybe you need to set the universal=1 option in setup.cfg?

  • Not really a bug, but I found the semantics of remove and discard surprising: I would have expected them to be the inverse of add, rather than removing/discarding all copies by default. Maybe a less surprising design would be to have remove remove 1 item, discard discard 0 or 1 items, and then have remove_all and discard_all methods for when you want to do that? I don't know if it's worth breaking backwards compatibility over though.

wheerd added a commit that referenced this issue Jun 16, 2017
@wheerd
Copy link
Owner

wheerd commented Jun 16, 2017

Thanks a lot for your feeback! I am happy that someone took the time to look through the code :-)
I fixed the issues you mentioned and will release a new version soon.

I am not sure about your last point. While I can see how this might be confusing, I do not want to make breaking changes to the API here. I don't think that the impact is that grave, especially since those methods can also be used to explicitly only remove/discard one element, e.g. with remove(1).

@wheerd
Copy link
Owner

wheerd commented Jun 16, 2017

I released it as version 2.0.1.

@wheerd wheerd closed this as completed Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants