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

Shortcut operators for union, intersection and difference #144

Merged
merged 7 commits into from Jun 2, 2020

Conversation

azmeuk
Copy link
Member

@azmeuk azmeuk commented May 29, 2020

Fixes #142
Implementation of __sub__, __or__ and __and__ as shortcuts for difference, union and intersection.

This makes the usage of BTrees datastructures closer to native python sets, that support those operators.

@azmeuk azmeuk changed the title Shortcut operators for Shortcut operators for union, intersection and difference May 29, 2020
BTrees/tests/common.py Outdated Show resolved Hide resolved
@azmeuk azmeuk force-pushed the issue-142-bitwise-operators branch 2 times, most recently from 6b6352e to 52946fb Compare June 1, 2020 11:03
@azmeuk azmeuk force-pushed the issue-142-bitwise-operators branch from 52946fb to 5f15484 Compare June 2, 2020 07:11
Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

This is looking pretty good to me! Thank you!

I think the only things missing are a change note about the added operators and a mention in the documentation. I think we should say what types support these operations with what sort of arguments — it's not entirely clear to me. It's probably fine to just refer to the original functions. Our sphinx configuration for autodoc does document special members, but I don't see any docstrings for these. It may be best to add them to the interfaces and document them there.

The number protocols methods ``__and__``, ``__or__`` and ``__sub__`` are provided
by all the data structures. They are shortcuts for :meth:`~BTrees.Interfaces.IMerge.intersection`,
:meth:`~BTrees.Interfaces.IMerge.union` and :meth:`~BTrees.Interfaces.IMerge.difference`.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just set this line of documentation because I was not sure on which interface to add the new operators. Is it ok like this? Or should I add the operators to the IBTree, ITreeSet and ISet interfaces?

Copy link
Member

Choose a reason for hiding this comment

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

It's the modules (e.g., BTrees.OOBTree) that implement IMerge, so I don't think the method definitions of the operators belong here. It doesn't hurt anything to mention in the main docstring here that there are operator synonyms for the functions defined by this interface, but I think that the operator method definitions should be moved to the appropriate datatype interfaces.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, just to be sure, IBTree, ITreeSet and ISet ?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds right to me. (I don't think there's an IBucket.)

@mgedmin
Copy link
Member

mgedmin commented Jun 2, 2020

All the OS X builders failed because

Checking dist/BTrees-4.7.3.dev0-cp38-cp38-macosx_10_9_x86_64.whl: FAILED
  `long_description` has syntax errors in markup and would not be rendered on PyPI.
    line 42: Error: Unknown interpreted text role "meth".
  warning: `long_description_content_type` missing.  defaulting to `text/x-rst`.

no Sphinx markup is allowed in README.rst and CHANGES.rst because those are used for long_description.

(In the past I've sometimes resorted to a judicious str.replace() to overcome this problem.)

@@ -5,7 +5,11 @@
4.7.3 (unreleased)
==================

- Nothing changed yet.
- BTrees, TreeSet, Set and Buckets implements the ``__and__``,
Copy link
Member

Choose a reason for hiding this comment

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

👍

@jamadden
Copy link
Member

jamadden commented Jun 2, 2020

Since the long_description error is just coming from CHANGES.rst directly, I personally would probably just remove the :meth: tags and just turn the references into double-backtick code.

@azmeuk
Copy link
Member Author

azmeuk commented Jun 2, 2020

Everything should be good now :)

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you!

I just have two minor nits.

BTrees/Interfaces.py Outdated Show resolved Hide resolved
CHANGES.rst Outdated Show resolved Hide resolved
@azmeuk azmeuk merged commit 74f01d5 into zopefoundation:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement bitwise operators
3 participants