Skip to content
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

_decimal difference with _pydecimal #73720

Closed
arigo mannequin opened this issue Feb 11, 2017 · 14 comments
Closed

_decimal difference with _pydecimal #73720

arigo mannequin opened this issue Feb 11, 2017 · 14 comments
Labels
3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error

Comments

@arigo
Copy link
Mannequin

arigo mannequin commented Feb 11, 2017

BPO 29534
Nosy @arigo, @rhettinger, @mdickinson, @skrah, @andrewnester
PRs
  • bpo-29534: _decimal difference with _pydecimal #65
  • [Do Not Merge] Sample of CPython life with blurb. #703
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-02-14.19:21:21.159>
    created_at = <Date 2017-02-11.13:54:15.001>
    labels = ['type-bug', '3.7']
    title = '_decimal difference with _pydecimal'
    updated_at = <Date 2017-03-31.17:09:42.520>
    user = 'https://github.com/arigo'

    bugs.python.org fields:

    activity = <Date 2017-03-31.17:09:42.520>
    actor = 'skrah'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-02-14.19:21:21.159>
    closer = 'mark.dickinson'
    components = []
    creation = <Date 2017-02-11.13:54:15.001>
    creator = 'arigo'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 29534
    keywords = []
    message_count = 14.0
    messages = ['287600', '287612', '287615', '287677', '287680', '287681', '287682', '287683', '287687', '287689', '287788', '287790', '287793', '290925']
    nosy_count = 5.0
    nosy_names = ['arigo', 'rhettinger', 'mark.dickinson', 'skrah', 'andrewnester']
    pr_nums = ['65', '703', '552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29534'
    versions = ['Python 3.5', 'Python 3.7']

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 11, 2017

    A difference in behavior between _decimal and _pydecimal (it seems that _decimal is more consistent in this case):

        class X(Decimal):
            def __init__(self, a):
                print('__init__:', a)
        X.from_float(42.5)   # __init__: Decimal('42.5')
    
        X.from_float(42)     # with _pydecimal: __init__: 42
                             # with _decimal:   __init__: Decimal('42')

    @arigo arigo mannequin added 3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error labels Feb 11, 2017
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Feb 11, 2017

    I get the the same output, perhaps I misunderstand something here:

    >>> from _decimal import *
    >>> class X(Decimal):
    ...         def __init__(self, a):
    ...             print('__init__:', a)
    ... 
    >>> X.from_float(42.5)
    __init__: 42.5
    Decimal('42.5')
    >>> X.from_float(42)
    __init__: 42
    Decimal('42')
    >>> 
    >>> 
    >>> from _pydecimal import *
    >>> class X(Decimal):
    ...         def __init__(self, a):
    ...             print('__init__:', a)
    ... 
    >>> X.from_float(42.5) 
    __init__: 42.5
    Decimal('42.5')
    >>> X.from_float(42) 
    __init__: 42
    Decimal('42')

    @arigo
    Copy link
    Mannequin Author

    arigo mannequin commented Feb 11, 2017

    Sorry! It should be repr(a) inside the print. Here is the fixed version:

        class X(Decimal):
            def __init__(self, a):
                print('__init__:', repr(a))
        X.from_float(42.5)   # __init__: Decimal('42.5')
    
        X.from_float(42)     # with _pydecimal: __init__: 42
                             # with _decimal:   __init__: Decimal('42')

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 13, 2017

    I've just added PR for this issue.

    @mdickinson
    Copy link
    Member

    Is there an observable difference in user behaviour if no subclassing of Decimal is involved? Or does this just affect Decimal subclasses?

    @mdickinson
    Copy link
    Member

    BTW, as a user, I'd expect Decimal.from_float to simply coerce its input to float if given an input that isn't actually a float in the first place; the fact that it has special handling in place for int inputs (and that that special handling takes the form of an isinstance check, rather than something more duck-typed) is surprising.

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 13, 2017

    actually, it's more related to subclassing, because the problem comes from the fact that before the fix __init__ method receives value as int not Decimal if int passed to from_float call.
    That's why only subclasses of Decimal can see difference in arguments received by __init__

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 13, 2017

    Agree about surprising behaviour but I guess it's better to fix it as other issue because it could break BC in some cases. At least it needs to be investigated.

    For now I would like to stick with same behaviour for _decimal and _pydecimal

    @mdickinson
    Copy link
    Member

    [Andrew]

    it's better to fix it as other issue

    No, it's better not to "fix" it at all. :-) It's one of those many behaviour changes where the answer to "Should we have done this differently in the first place" might possibly be "Yes" (and might also be "No"), but the answer to "Should we *change* it now" is a definite "No" (especially given that the "from_float" method is there almost entirely for backwards compatibility). I shouldn't have brought it up. Apologies.

    @andrewnester
    Copy link
    Mannequin

    andrewnester mannequin commented Feb 13, 2017

    thanks for your notes, it's absolutely fine and I agree with you :)

    @mdickinson
    Copy link
    Member

    PR merged; thank you!

    Unfortunately, just after I merged it I noticed that the Misc/NEWS entry was in the wrong section. I'll make a new PR to fix that, and close this issue once it's done.

    @mdickinson
    Copy link
    Member

    PR to fix the Misc/NEWS entry: #99

    @mdickinson
    Copy link
    Member

    All done. Closing. (In theory, as a bugfix, this could be backported to 3.5 and 3.6. In practice, I doubt it's worth the effort.)

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 31, 2017

    Does anyone know how to disable the spurious pull requests that keep coming in?

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant