bpo-29751: DOC: PyLong_FromString wrong on number with leading zero a…#915
Conversation
|
@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. |
| 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 |
There was a problem hiding this comment.
Expand sentence to
... Leading spaces and single underscores after a base specifier and between digits are
ignored.
|
I think the underscore change is only for 3.7 and 3.6 based on the note on the literals page. |
3121d3a to
7cc682f
Compare
|
Sorry about that. I think it's OK now? |
|
LGTM. Thanks. |
| ``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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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”.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I found the :token: in the docs to link to decinteger. I'll work on the wording.
| 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`. |
There was a problem hiding this comment.
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, . . .
|
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. |
(cherry picked from commit 26896f2)
(cherry picked from commit 26896f2)
…nd base=0