-
-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
bpo-16011: Clarify behavior of __contains__ in user defined classes #152
Conversation
aktech
commented
Feb 18, 2017
•
edited
Loading
edited
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:
Thanks again to your contribution and we look forward to looking at it! |
Doc/reference/expressions.rst
Outdated
For user-defined classes which define the :meth:`__contains__` method, ``x in | ||
y`` is true if and only if ``y.__contains__(x)`` is true. | ||
For user-defined classes which define the :meth:`__contains__` method, the | ||
``in`` operator coerces the result returned by :meth:`__contains__` to a boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence should be ended with a full stop. Or if you used comma intentionally, "Hence" shouldn't begin with an uppercase letter "H".
Doc/reference/expressions.rst
Outdated
y`` is true if and only if ``y.__contains__(x)`` is true. | ||
For user-defined classes which define the :meth:`__contains__` method, the | ||
``in`` operator coerces the result returned by :meth:`__contains__` to a boolean, | ||
Hence ``x in y`` is true if and only if ``y.__contains__(x)`` is true. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps changing the example to bool(y.__contains__(x))
would make the whole paragraph clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Otherwise I don't see the difference with the original one.
I feel proposed sentence is unnecessary long. How about this?
|
I have put the full stop before
|
Can we merge this now? |
You know what, reading this change side by side with the original, I think I'd instead favor making it:
Throughout the section, we should then change 'true' to |
@bitdancer does that looks good now? |
Yes, but we should also change 'true' to |
Yes, Indeed. I have changed that now. |
@bitdancer Is this good to go now? |
Doc/reference/expressions.rst
Outdated
substring of *y*. An equivalent test is ``y.find(x) != -1``. Empty strings are | ||
always considered to be a substring of any other string, so ``"" in "abc"`` will | ||
return ``True``. | ||
|
||
For user-defined classes which define the :meth:`__contains__` method, ``x in | ||
y`` returns ``True`` if ``y.__contains__(x)`` returns a true value, and | ||
y`` returns ``True`` if ``y.__contains__(x)`` returns a ``True`` value, and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second True
here should be just true.
@bitdancer Can we merge this now? |
Not until the Travis check passes. I don't know what's wrong with that or how to get it fixed, unfortunately. |
290b63f
to
31fa785
Compare
@ncoghlan Can you help me with this? Need to restart the travis build for this PR. |
@aktech Since there isn't even a "Details" link, I can't trigger a rebuild from the Travis side of things, but you should be able to push an empty commit to the PR branch as described in http://stackoverflow.com/questions/17606874/trigger-a-travis-ci-rebuild-without-pushing-a-commit/31694534#31694534 Since we use the squash-and-merge workflow, that won't show up in the eventual commit history. |
Maybe try rebasing with upstream/master and push again? |
Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint. |
Thanks @aktech for the PR, and @bitdancer for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7. |
Sorry, @aktech and @bitdancer, I could not cleanly backport this to |
Preparations: - use meaningful variable names - remove dead code - move the stackless addition to PyThreadState to the end of the struct - remove redundant declarations from stackless_structs.h
Add legitimate globals to ignored-globals.txt. All other Stackless globals should be moved to runtime or interpreter state.
It was only used during module initialization.
…time and interpreter state Add struct _stackless_runtime_state to _PyRuntimeState and PyStacklessInterpreterState to PyInterpreterState. For now both structs are empty. I'll move Stackless globals into them.
Add sub-struct transfer to _PyRuntime.st.
…essInterpreterState The variable holds a complex callable object, which must not be shared between different interpreters.
Preparations: - use meaningful variable names - remove dead code - move the stackless addition to PyThreadState to the end of the struct - remove redundant declarations from stackless_structs.h
Add legitimate globals to ignored-globals.txt. All other Stackless globals should be moved to runtime or interpreter state.
It was only used during module initialization.
…time and interpreter state Add struct _stackless_runtime_state to _PyRuntimeState and PyStacklessInterpreterState to PyInterpreterState. For now both structs are empty. I'll move Stackless globals into them.
Add sub-struct transfer to _PyRuntime.st.
…essInterpreterState The variable holds a complex callable object, which must not be shared between different interpreters.
Removed globals: - slp_initial_tstate - channel_hook - slp_error_handler - _slp_schedule_fasthook - _slp_schedule_hook - slp_cstack_chain - mem_bomb Additionally I optimized the ordering of the fields of PyStacklessState and PyStacklessInterpreterState to avoid padding.
Move slp_enable_softswitch to PyStacklessInterpreterState. Move slp_try_stackless, cstack_cache and cstack_cachecount to struct _stackless_runtime_state.
Introduced by commit 6e1250a / issue python#152.
The commit used a C11 feature. Unfortunately only clang emits a warning.