FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-30940: fix docs for round if second arg is None by daxlab · Pull Request #2824 · python/cpython · GitHub

/ cpython Public

bpo-30940: fix docs for round if second arg is None - #2824

Closed
daxlab wants to merge 4 commits into
python:masterfrom
daxlab:bpo_30940
ConversationCommits (4)ChecksFiles changed
Closed

bpo-30940: fix docs for round if second arg is None#2824
daxlab wants to merge 4 commits into
python:masterfrom
daxlab:bpo_30940

Conversation

daxlab commented Jul 23, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

mention-bot commented Jul 23, 2017

Copy link
Copy Markdown

@daxlab, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @benjaminp and @rhettinger to be potential reviewers.

serhiy-storchaka reviewed Jul 23, 2017
View reviewed changes
Comment thread Doc/library/functions.rst Outdated
``2``). Any integer value is valid for *ndigits* (positive, zero, or
negative). The return value is an integer if called with one argument,
otherwise of the same type as *number*.
negative). The return value is an integer if *ndigits* is omitted or *None*.

serhiy-storchaka Jul 23, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Format None with fixed-width font instead of italic.

serhiy-storchaka requested changes Jul 23, 2017
View reviewed changes

serhiy-storchaka 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Needed to update the docstring too.

The signature of the function can be written as

round(number, ndigits=None)

serhiy-storchaka added needs backport to 3.5 docs Documentation in the Doc dir labels Jul 23, 2017
mdickinson approved these changes Jul 23, 2017
View reviewed changes

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM. Thanks for doing this!

serhiy-storchaka commented Jul 23, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Please update the docstring.

And what is more correct,

if *ndigits* is omitted or is ``None``

(as in the first paragraph), or

if *ndigits* is omitted or ``None``

(as in the second paragraph)? Or both are correct?

daxlab commented Jul 23, 2017

Copy link
Copy Markdown
Contributor Author

if *ndigits* is omitted or ``None`` looks correct.

serhiy-storchaka reviewed Jul 23, 2017
View reviewed changes
Comment thread Python/bltinmodule.c Outdated
"round(number, ndigits=None) -> number\n\
\n\
Round a number to a given precision in decimal digits (default 0 digits).\n\
This returns an int when called with one argument, otherwise the\n\

serhiy-storchaka Jul 23, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"if ndigits is omitted or None"

mdickinson commented Jul 23, 2017

Copy link
Copy Markdown
Member

About

if ndigits is omitted or is None

versus

if ndigits is omitted or None

I think either is fine. To my eyes, the first looks a bit clearer, but also a bit more pedantic.

daxlab commented Jul 24, 2017

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka docs updated.

daxlab commented Aug 27, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka plz review. Also, should I send a separate PR to 3.6 branch or you can cherry-pick ?

serhiy-storchaka commented Aug 27, 2017

Copy link
Copy Markdown
Member

The current changes LGTM, but see a discussion on the tracker. Perhaps the documentation needs additional clarification.

ned-deily commented Jun 8, 2018

Copy link
Copy Markdown
Member

Based on more recent discussion on the tracker, it looks these proposed changes have now been addressed by other PRs, primarily #6342 and #2740. Thanks for starting the ball rolling @daxlab!

ned-deily closed this Jun 8, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

serhiy-storchaka serhiy-storchaka approved these changes
+1 more reviewer
mdickinson mdickinson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting merge docs Documentation in the Doc dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL