bpo-30085: Improve documentation for operator #1171
Conversation
|
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 the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
|
@SanketDG, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @benjaminp and @tiran to be potential reviewers. |
Doc/library/operator.rst
Outdated
There was a problem hiding this comment.
Remote the .. note: directive and combine the two sentences into one paragraph. That will get the message across but not litter the visual appearance of the docs with big note blocks that aren't central to actually using the module. In particular, this note is of less documentation value than the following paragraph that describes the function categories. It would be better if all of that text (the intro paragraph, the two new sentences, and the categorization paragraph were read as a single block of text with three paragraphs. Breaking it up with note blocks impairs intelligiblity and created a weird emphasis on what not to do rather than what to do (see the dev guide suggestions for documenting affirmatively).
There was a problem hiding this comment.
Thank you for the constructive comment, it does make sense to not use note, because it denotes a certain emphasis.
I have updated it to be read as a paragraph.
Doc/library/operator.rst
Outdated
There was a problem hiding this comment.
Should "without" be "with"?
There was a problem hiding this comment.
Yes, I have fixed that and also removed the word also since it doesn't stand in this context.
Thanks!
terryjreedy
left a comment
terryjreedy
left a comment
There was a problem hiding this comment.
See the issue for suggested revision, which is close to this one after the requested revisions.
Doc/library/operator.rst
Outdated
Doc/library/operator.rst
Outdated
There was a problem hiding this comment.
and delete promises about the future.
|
ping @terryjreedy @rhettinger, this looks good? |
|
I presume 'this' is 6ca4493. Sanket, apparently made a 'force-push', whereas simple commits are not recommended in the devguide. On the other hand, it makes the final net patch visible. |
Doc/library/operator.rst
Outdated
There was a problem hiding this comment.
Please change "We recommend using the dunderless form." to "The dunderless form is preferred for clarity". Normally, we don't use "we" in the docs :-) Also, let's drop the last sentence. It is confusing and isn't essential to the message. The cases were they aren't the same are somewhat esoteric (i.e. doesn't check NotImplemented with a fallback to the y.radd(x) method). Otherwise, for most intents and purposes they are the same.
There was a problem hiding this comment.
Dunderless sounds like a made-up term or jargon. I don’t think the documentation even uses dunder anywhere. I would write it as “the variants without the double underscores”.
|
@rhettinger @vadmium I have updated the same, thanks! |
Codecov Report
@@ Coverage Diff @@
## master #1171 +/- ##
==========================================
- Coverage 83.7% 82.68% -1.02%
==========================================
Files 1369 1430 +61
Lines 346453 352806 +6353
==========================================
+ Hits 289991 291728 +1737
- Misses 56462 61078 +4616 |
|
@rhettinger ping. |
|
@rhettinger It's been a while 😄 |
Prefer dunderless functions, and mention that the dunder functions only exist because of the sake of backward compatilibity.
|
@rhettinger @vadmium @terryjreedy Can I get some update on this? |
|
Raymond, you assigned this to yourself. I think all review requests have been met. Do you want to write the news blurb, have me write one (and merge), or skip the news? |
|
@terryjreedy this is ready for merge? Any changes needed? |
|
@rhettinger ping |
|
Hmm, Is it allowed that someone else merge it when the original assignee is not merging it? |
|
Thanks @SanketDG for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6. |
|
GH-3736 is a backport of this pull request to the 3.6 branch. |
The dunderless functions are preferred; dunder are retained for back compatilibity. Patch by Sanket Dasgupta. (cherry picked from commit 5b9299d)
10 participants
Prefer dunderless functions, and mention that the dunder
functions only exist because of the sake of backward
compatilibity.
https://bugs.python.org/issue30085