Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-102431: clarify constraints on arguments of logical_xxx methods#102836

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
serhiy-storchaka merged 14 commits intopython:mainfromskirpichev:decimal-logop-docs
Oct 14, 2025

Conversation

@skirpichev
Copy link
Contributor

@skirpichevskirpichev commentedMar 20, 2023
edited by bedevere-bot
Loading

@skirpichev
Copy link
ContributorAuthor

BTW, maybe it does make sense to addis_logical() method (there is_islogical() for pure-Python version) to test constraints for this type of decimal's. Probably, that could simplify documentation, esp. docstrings.

@rhettinger
Copy link
Contributor

I would go farther an include the full text and examples used inLib/_pydecimal.py:

    def logical_and(self, a, b):        """Applies the logical operation 'and' between each operand's digits.        The operands must be both logical numbers.        >>> ExtendedContext.logical_and(Decimal('0'), Decimal('0'))        Decimal('0')        >>> ExtendedContext.logical_and(Decimal('0'), Decimal('1'))        Decimal('0')        >>> ExtendedContext.logical_and(Decimal('1'), Decimal('0'))        Decimal('0')        >>> ExtendedContext.logical_and(Decimal('1'), Decimal('1'))        Decimal('1')        >>> ExtendedContext.logical_and(Decimal('1100'), Decimal('1010'))        Decimal('1000')        >>> ExtendedContext.logical_and(Decimal('1111'), Decimal('10'))        Decimal('10')        >>> ExtendedContext.logical_and(110, 1101)        Decimal('100')        >>> ExtendedContext.logical_and(Decimal(110), 1101)        Decimal('100')        >>> ExtendedContext.logical_and(110, Decimal(1101))        Decimal('100')        """        a = _convert_other(a, raiseit=True)        return a.logical_and(b, context=self)

Anything less that this will just leave the user guessing about how to use these weird functions.

@skirpichev
Copy link
ContributorAuthor

I would go farther an include the full text and examples used in Lib/_pydecimal.py:

Agreed. But there should be some formal description too, because "The operands must be both logical numbers." sounds cryptic even after examples... Or do you think it's ok? (The notion of "logical number" is well defined in the sphinx docs.)

I'll sync docstrings for C/Python implementations and add examples.

@skirpichev
Copy link
ContributorAuthor

Ok, now docstrings of Context's methods are in sync.
But, maybe we could reduce the number of examples? E.g. in thelogical_and case to:

>>> ExtendedContext.logical_and(Decimal('0'), Decimal('0'))Decimal('0')>>> ExtendedContext.logical_and(Decimal('0'), Decimal('1'))Decimal('0')>>> ExtendedContext.logical_and(Decimal('1'), Decimal('1'))Decimal('1')>>> ExtendedContext.logical_and(Decimal('1111'), Decimal('10'))Decimal('10')

Basically, this shows everything: constraints on digits of operands, how it works digits-wise and how it works with coefficients ofdifferent length. I think it should reduce misunderstanding to minimum.

@skirpichev
Copy link
ContributorAuthor

@rhettinger, does it make sense for you now?

@skirpichev
Copy link
ContributorAuthor

CC@picnixz
CC@serhiy-storchaka

@picnixzpicnixz self-requested a reviewFebruary 24, 2025 20:10
Copy link
Member

@picnixzpicnixz left a comment

Choose a reason for hiding this comment

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

I think this makes sense to have the inline definition of what a logical operand is but OTOH, one could say that the online docs should be sufficient so I'm neutral on this.

@serhiy-storchaka
Copy link
Member

Sorry, I am not a Decimal expert and do not know why such weird operations exist at first place (perhaps it is just the part of some standard).

From practical point of view, I would just refer "logical integers" in the docstrings, and defined them in the module documentation. If you want to add examples in the docstrings and if there are examples in other docstring, I think that a single example per method should be enough. With multiple digits you can cover all cases by a single example. The rest can be added in the module documentation.

@skirpichev
Copy link
ContributorAuthor

perhaps it is just the part of some standard

@serhiy-storchaka, sure. If you see something odd in the decimal module - IMO, it's coming from the IBM standard. JFR:https://speleotrove.com/decimal/damisc.html

If you want to add examples in the docstrings and if there are examples in other docstring, I think that a single example per method should be enough.

Current patch just sync Python (which has extensive examples for context functions) and C implementation. Do you think that we should remove (most) examples from pure-Python version?

@serhiy-storchaka
Copy link
Member

Well, then we should keep examples. They are not just the documentation, but doctests.

But new additions are incorrect. "Both operands should have zero sign and exponent, and digits either 0 or 1." Operands can be integers, and integers don't have exponent or digits. Lets leave a more detailed explanation to the module documentation.

@skirpichev
Copy link
ContributorAuthor

Operands can be integers, and integers don't have exponent or digits.

I doubt we should be more verbose here (docstrings!). And I don't think that sphinx docs needs to be expanded. Context docs alreadyassume that integers are coerced to Decimal's: "EachContext method accepts a Python integer (an instance ofint) anywhere that a Decimal instance is accepted."

@serhiy-storchaka
Copy link
Member

I also do not think that we should be more verbose here.

@skirpichev
Copy link
ContributorAuthor

CC@vstinner

@skirpichev
Copy link
ContributorAuthor

PR updated after recent transition of the C decimal module to AC, please review.

Python and C docstrings are in sync. Tiny difference is indentation of examples for C version (for consistency with other docstrings).

Former Python docstrings (not all) have remarks like "The operands must be both logical numbers." We can use that instead of verbose "Both operands should have zero sign and exponent, and digits either 0 or 1." Notion of "logical number" is explained in sphinx docs.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@serhiy-storchaka
Copy link
Member

Yes, please keep old remarks.

Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

LGTM. I'm fine with "The operands must be both logical numbers."

@skirpichev
Copy link
ContributorAuthor

@serhiy-storchaka, done

Copy link
Member

@serhiy-storchakaserhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@serhiy-storchakaserhiy-storchaka added needs backport to 3.14bugs and security fixes needs backport to 3.13bugs and security fixes labelsOct 14, 2025
@serhiy-storchakaserhiy-storchakaenabled auto-merge (squash)October 14, 2025 10:45
@serhiy-storchakaserhiy-storchaka merged commit6ecf77d intopython:mainOct 14, 2025
49 checks passed
@miss-islington-app
Copy link

Thanks@skirpichev for the PR, and@serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Sorry,@skirpichev and@serhiy-storchaka, I could not cleanly backport this to3.14 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 6ecf77dbdec7838e9ce2298cb8d16e8c2250da81 3.14

@miss-islington-app
Copy link

Sorry,@skirpichev and@serhiy-storchaka, I could not cleanly backport this to3.13 due to a conflict.
Please backport usingcherry_picker on command line.

cherry_picker 6ecf77dbdec7838e9ce2298cb8d16e8c2250da81 3.13

@skirpichev
Copy link
ContributorAuthor

I'll provide backports.

vstinner reacted with hooray emoji

@skirpichevskirpichev deleted the decimal-logop-docs branchOctober 14, 2025 12:06
skirpichev added a commit to skirpichev/cpython that referenced this pull requestOct 14, 2025
…gical operations (pythonGH-102836)Sync C/Python implementation of the decimal: logical_ops for contexts.(cherry picked from commit6ecf77d)Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@bedevere-app
Copy link

GH-140105 is a backport of this pull request to the3.14 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.14bugs and security fixes labelOct 14, 2025
skirpichev added a commit to skirpichev/cpython that referenced this pull requestOct 14, 2025
…gical operations (pythonGH-102836)Sync C/Python implementation of the decimal: logical_ops for contexts.(cherry picked from commit6ecf77d)Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
@bedevere-app
Copy link

GH-140106 is a backport of this pull request to the3.13 branch.

@bedevere-appbedevere-appbot removed the needs backport to 3.13bugs and security fixes labelOct 14, 2025
vstinner pushed a commit that referenced this pull requestOct 14, 2025
…operations (GH-102836) (#140105)* [3.14]gh-102431: Clarify constraints on operands of Decimal logical operations (GH-102836)Sync C/Python implementation of the decimal: logical_ops for contexts.(cherry picked from commit6ecf77d)Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
vstinner pushed a commit that referenced this pull requestOct 14, 2025
…operations (GH-102836) (#140106)* [3.13]gh-102431: Clarify constraints on operands of Decimal logical operations (GH-102836)Sync C/Python implementation of the decimal: logical_ops for contexts.(cherry picked from commit6ecf77d)Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@vstinnervstinnervstinner approved these changes

@serhiy-storchakaserhiy-storchakaserhiy-storchaka approved these changes

@picnixzpicnixzpicnixz approved these changes

@rhettingerrhettingerAwaiting requested review from rhettinger

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@skirpichev@rhettinger@serhiy-storchaka@vstinner@picnixz@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp