Skip to content

bpo-29751: DOC: PyLong_FromString wrong on number with leading zero a…#915

Merged
Mariatta merged 7 commits into
python:masterfrom
csabella:doc_changes
Apr 24, 2017
Merged

bpo-29751: DOC: PyLong_FromString wrong on number with leading zero a…#915
Mariatta merged 7 commits into
python:masterfrom
csabella:doc_changes

Conversation

@csabella

Copy link
Copy Markdown
Contributor

…nd base=0

@mention-bot

Copy link
Copy Markdown

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

Comment thread Doc/c-api/long.rst Outdated
radix 2 will be used; otherwise radix 10 will be used. If *base* is not
radix 2 will be used; if *str* starts with any number other than ``0``, then
radix 10 will be used; otherwise :exc:`ValueError` will be raised. If *base* is not
``0``, it must be between ``2`` and ``36``, inclusive. Leading spaces are

@terryjreedy terryjreedy Mar 30, 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.

Expand sentence to
... Leading spaces and single underscores after a base specifier and between digits are
ignored.

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.

@csabella

Copy link
Copy Markdown
Contributor Author

I think the underscore change is only for 3.7 and 3.6 based on the note on the literals page.

@abalkin

abalkin commented Apr 1, 2017

Copy link
Copy Markdown
Member

@csabella - your PR includes commit 4a351fa50c682845d2baedcf167a9e3103095f8c from bpo-28157. You can remove it by running git rebase -i origin/master.

@csabella csabella force-pushed the doc_changes branch 2 times, most recently from 3121d3a to 7cc682f Compare April 1, 2017 21:40
@csabella

csabella commented Apr 1, 2017

Copy link
Copy Markdown
Contributor Author

Sorry about that. I think it's OK now?

@abalkin

abalkin commented Apr 1, 2017

Copy link
Copy Markdown
Member

LGTM. Thanks.

Comment thread Doc/c-api/long.rst Outdated
``0``, it must be between ``2`` and ``36``, inclusive. Leading spaces are
ignored. If there are no digits, :exc:`ValueError` will be raised.
radix 2 will be used; if *str* starts with any number other than ``0``, then
radix 10 will be used; otherwise :exc:`ValueError` will be raised. If *base* is not

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.

This seems to have a double negative. Why not write “. . . if *str* starts with the digit 0, a :exc:`ValueError` will be raised; otherwise, radix 10 will be used.”?

But this definition seems suspect. Can you pass in strings representing zero? See the other documentation at https://docs.python.org/3/reference/lexical_analysis.html#integers.

@csabella csabella Apr 2, 2017

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.

If you send in a string representing zeroes with a base of 0, then it returns 0 because it keeps the same base (that is, keeps base 0). I didn't know if that was special enough to include it as another option in the sentence. The reason I put the "any number other than 0" was because I thought the exception condition might need to be last. If not, I'll reword it. The whole sentence is a lot of information as it is. Do you think bullet points or some other structure might simplify it? It also seems like a visual representation would be easier than all the words.

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.

I don’t think forcing the ValueError case to the end is that important, and I though the text “if . . . [the prefix is] other than 0 . . .; otherwise” is harder to understand than it could be. But this is only a minor thing though; if you think putting ValueError at the end is important, so be it :)

Bullet points might simplify it; that did cross my mind. Another option would be to drop the details and just link to the Lexical Analysis section for the details. That would eliminate the problem with a string like "000" not “starting with a number other than 0”, yet presumably this doesn’t raise ValueError.

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.

I've linked to the Lexical Analysis, but I couldn't figure out how to link to the decinteger part of that (it's showing in italics now instead of a link). I feel like this is headed in the right direction, but maybe not quite there yet. Looking forward to your feedback.

Comment thread Doc/c-api/long.rst Outdated
ignored. If there are no digits, :exc:`ValueError` will be raised.
representation of the number. If *base* is ``0``, the *str* is interpreted using
the :ref:`integers` defintion; additionally, a literal 0 evaluates as *base* 0 and
a :attr:`decinteger` beginning with 0 raises a :exc:`ValueError`. If *base* is not

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.

The rest of the text refers to the first argument by name as *str*, not “the *str*”, so I would drop “the” and just write “If *base* is 0, *str* is interpreted . . .”

Spelling: defintion → definition.

What does “a literal 0 evaluates as *base* 0” mean? This is already qualified with “if *base* is 0”.

I understand the “:attr:” markup is supposed to be for class attributes. Is “decinteger” a reference to the grammar definition:

decinteger: nonzerodigit (["_"] digit)* | "0"+ (["_"] "0")*

You may have to add more text to make this obvious, or explain what you mean by one “beginning with 0”.

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!

The 'literal 0 evaluates as base 0 was for the case mentioned in an earlier comment where any str of zero (eg"0", "000000000000") gives back 0 as the result. It doesn't follow the other rules on the Lexical Analysis page just as the 'ValueError' for the decintegers isn't on that page. The lexical analysis page shows that the base 0 is converted to another base according to the first two characters in str, so no other str is evaluated as base 0. Maybe I should change it to 'a literal 0 evaluates as radix 0'?

I didn't want to use the :attr: on the decinteger. I just couldn't figure out what it should be in order to link to that definition on the other page. I had it as a :ref: and that didn't work. Honestly, I changed it to :attr: when my Travis build failed and I couldn't figure out why. As you asked, it is supposed to be a reference to the grammar definition.

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.

I found the :token: in the docs to link to decinteger. I'll work on the wording.

Comment thread Doc/c-api/long.rst Outdated
ignored. If there are no digits, :exc:`ValueError` will be raised.
representation of the number. If *base* is ``0``, *str* is interpreted using
the :ref:`integers` definition; a literal 0 evaluates to 0; since
leading zeroes are not allowed in a :token:`decinteger`, this raises a :exc:`ValueError`.

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.

The second and third parts about literal 0 and leading zeros still seem awkward. What about

If *base* is 0, *str* is interpreted using the :ref:`integers` definition. In this case, leading zero digits followed by a non-zero decimal digit raises a :exc:`ValueError`. If *base* is not 0, . . .

@csabella

Copy link
Copy Markdown
Contributor Author

I made the changes you suggested. I used the wording about the leading zeros exactly as it is on the Integer literals definition. Sorry about the wording on the 'literal 0', I had thought you asked for that in an earlier comment, so I kept trying to fit it in.

@Mariatta Mariatta merged commit 26896f2 into python:master Apr 24, 2017
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 24, 2017
Mariatta pushed a commit to Mariatta/cpython that referenced this pull request Apr 24, 2017
Mariatta added a commit that referenced this pull request Apr 24, 2017
Mariatta added a commit that referenced this pull request Apr 24, 2017
@csabella csabella deleted the doc_changes branch April 24, 2017 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants