Skip to content

bpo-30085: Improve documentation for operator#1171

Merged
terryjreedy merged 2 commits into
python:masterfrom
SanketDG:issue-30085
Sep 24, 2017
Merged

bpo-30085: Improve documentation for operator#1171
terryjreedy merged 2 commits into
python:masterfrom
SanketDG:issue-30085

Conversation

@SanketDG

@SanketDG SanketDG commented Apr 18, 2017

Copy link
Copy Markdown
Contributor

Prefer dunderless functions, and mention that the dunder
functions only exist because of the sake of backward
compatilibity.

https://bugs.python.org/issue30085

@the-knights-who-say-ni

Copy link
Copy Markdown

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!

@mention-bot

Copy link
Copy Markdown

@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.

Comment thread Doc/library/operator.rst Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@rhettinger rhettinger self-assigned this Apr 18, 2017
Comment thread Doc/library/operator.rst Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should "without" be "with"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, I have fixed that and also removed the word also since it doesn't stand in this context.

Thanks!

@terryjreedy terryjreedy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

See the issue for suggested revision, which is close to this one after the requested revisions.

Comment thread Doc/library/operator.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove 'new paragraph'

Comment thread Doc/library/operator.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and delete promises about the future.

@SanketDG

Copy link
Copy Markdown
Contributor Author

ping @terryjreedy @rhettinger, this looks good?

@terryjreedy

Copy link
Copy Markdown
Member

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.
Raymond, I think this is ready to merge. I am not sure about backporting.

Comment thread Doc/library/operator.rst Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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”.

@SanketDG

SanketDG commented May 8, 2017

Copy link
Copy Markdown
Contributor Author

@rhettinger @vadmium I have updated the same, thanks!

@codecov

codecov Bot commented May 8, 2017

Copy link
Copy Markdown

Codecov Report

Merging #1171 into master will decrease coverage by 1.01%.
The diff coverage is n/a.

@@            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

@SanketDG

Copy link
Copy Markdown
Contributor Author

@rhettinger ping.

@SanketDG

SanketDG commented Jul 2, 2017

Copy link
Copy Markdown
Contributor Author

@rhettinger It's been a while 😄

Prefer dunderless functions, and mention that the dunder
functions only exist because of the sake of backward
compatilibity.
@SanketDG

Copy link
Copy Markdown
Contributor Author

@rhettinger @vadmium @terryjreedy Can I get some update on this?

@terryjreedy

Copy link
Copy Markdown
Member

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?

@SanketDG

Copy link
Copy Markdown
Contributor Author

@terryjreedy this is ready for merge? Any changes needed?

@tiran

tiran commented Sep 15, 2017

Copy link
Copy Markdown
Member

@rhettinger ping

@SanketDG

Copy link
Copy Markdown
Contributor Author

Hmm, Is it allowed that someone else merge it when the original assignee is not merging it?

@terryjreedy terryjreedy merged commit 5b9299d into python:master Sep 24, 2017
@SanketDG SanketDG deleted the issue-30085 branch September 24, 2017 20:49
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @SanketDG for the PR, and @terryjreedy for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@bedevere-bot

Copy link
Copy Markdown

GH-3736 is a backport of this pull request to the 3.6 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 24, 2017
The dunderless functions are preferred; dunder are retained for back compatilibity.
Patch by Sanket Dasgupta.
(cherry picked from commit 5b9299d)
terryjreedy pushed a commit that referenced this pull request Sep 24, 2017
The dunderless functions are preferred; dunder are retained for back compatilibity.
Patch by Sanket Dasgupta.
(cherry picked from commit 5b9299d)
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

Successfully merging this pull request may close these issues.

10 participants