-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-128185: Align the grammar of the Decimal
string constructor with float
's
#128315
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually tested something and saw that Decimal("NaN__")
is accepted as being a NaN. So, the grammar of NaN should also be changed. Initially, I thought that the grammar would be improved, but it might be easier to simply keep the mention in the text =/ (sorry @skirpichev, but you were probably right).
However, we can definitely use a production list instead.
Doc/library/decimal.rst
Outdated
exponentpart: indicator [sign] digits | ||
infinity: "Infinity" | "Inf" | ||
nan: "NaN" [`digits`] | "sNaN" [`digits`] | ||
numericvalue: `decimalpart` [`exponentpart`] | `infinity` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do Sphinx production lists support numeric-value
instead or should it be without any weird symbol? if not, use _
instead of -
to separate words.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx does not support -
, but it does support _
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yet another thing I should perhaps add to my list of things to fix upstream... Thanks for checking.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
The implementation is cpython/Modules/_decimal/_decimal.c Lines 2135 to 2139 in 2cf396c
If I am correct, this ignores every '_' in the string no matter where it is, similar to string.replace('_', '') . Decimal('__I__N__F__') and Decimal('__N__A__N__') are also valid. It is difficult to generalize an expression for NaN or Infinity.
EDIT: |
Considering this, I think we should keep the previous text. Sorry for this blunder but I didn't know that it also applied to |
OK. Should I keep the underscores in |
No, let's just revert the changes. It would be more confusing if we keep it in the grammar (especially considering how the pattern would actually read). Just transform the current grammar into a production list. However, before that, I'll ask some docs expert for their opinion @ncoghlan @willingc (the problem with me is that I wouldn't mind the change but we're already saying "we trim whitespaces" so we're already "pre-processing" the input to ease the grammar specifications). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it was already noted in the issue thread, I don't think there is a bug.
New docs are inaccurate, as already noted by Benedict. While it's possible to "fix" this, I doubt that will be an improvement: new grammar will be too complex, perhaps misleading and reveal implementation details, which should be rather hidden.
To summarize (unless our docs experts advise differently):
I would recommend merging #128323 first and then this one or do both in one PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, except for few grammar fixes.
Though, maybe we want instead reference terms from other grammars, like https://docs.python.org/3/reference/lexical_analysis.html#floating-point-literals does (almost everything is same as for grammar in the float() docs: https://docs.python.org/3/library/functions.html#float).
But lets see what @picnixz think on this first. Maybe for readability it's better to repeat all in decimal docs.
infinity: "Infinity" | "Inf" | ||
nan: "NaN" [`digits`] | "sNaN" [`digits`] | ||
numeric_value: `decimal_part` [`exponent_part`] | `infinity` | ||
numeric_string: [`sign`] `numeric_value` | [`sign`] `nan` | ||
|
||
Other Unicode decimal digits are also permitted where ``digit`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you can reference grammar term instead. Though, this may add a conflict with #128323, so - you can include that change here as well.
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
I don't have a strong preference. For instance, I personally prefer |
Yes that were my thoughts as well. I think it's ok to repeat grammar terms, even if they introduced before on other pages. |
This PR
digits
fromdigit [digit]...
to(digit | "_")* digit (digit | "_")*
.. productionlist::
Decimal
string constructor withfloat
's #128185📚 Documentation preview 📚: https://cpython-previews--128315.org.readthedocs.build/en/128315/library/decimal.html#decimal.Decimal