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

locale.atof documentation is missing func argument #58126

Closed
cedk mannequin opened this issue Feb 1, 2012 · 24 comments
Closed

locale.atof documentation is missing func argument #58126

cedk mannequin opened this issue Feb 1, 2012 · 24 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@cedk
Copy link
Mannequin

cedk mannequin commented Feb 1, 2012

BPO 13918
Nosy @birkenfeld, @terryjreedy, @mdickinson, @pitrou, @vstinner, @ezio-melotti, @cedk
Files
  • doc_atof.patch
  • delocalize.patch
  • delocalize.patch
  • delocalize.patch
  • 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 2014-10-23.20:53:30.100>
    created_at = <Date 2012-02-01.14:12:20.474>
    labels = ['type-feature', 'library']
    title = 'locale.atof documentation is missing func argument'
    updated_at = <Date 2014-10-23.21:04:24.618>
    user = 'https://github.com/cedk'

    bugs.python.org fields:

    activity = <Date 2014-10-23.21:04:24.618>
    actor = 'georg.brandl'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-10-23.20:53:30.100>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2012-02-01.14:12:20.474>
    creator = 'ced'
    dependencies = []
    files = ['29414', '36955', '36960', '36964']
    hgrepos = []
    issue_num = 13918
    keywords = ['patch']
    message_count = 24.0
    messages = ['152430', '152478', '152479', '184119', '184121', '184165', '184167', '184168', '184226', '184251', '184292', '184295', '184612', '229570', '229573', '229641', '229644', '229660', '229899', '229901', '229902', '229903', '229905', '229906']
    nosy_count = 9.0
    nosy_names = ['georg.brandl', 'terry.reedy', 'mark.dickinson', 'pitrou', 'vstinner', 'ezio.melotti', 'ced', 'docs@python', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue13918'
    versions = ['Python 3.5']

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Feb 1, 2012

    atof has a func argument used to instantiate the result but it is missing in the documentation.

    @cedk cedk mannequin assigned docspython Feb 1, 2012
    @cedk cedk mannequin added docs Documentation in the Doc dir type-feature A feature request or enhancement labels Feb 1, 2012
    @birkenfeld
    Copy link
    Member

    I don't think that argument needs to be documented. It's just there because somebody thought that copying 3 lines from atof into atoi was a bad idea.

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Feb 2, 2012

    Indeed I find it useful to use to get a Decimal instead of a float.
    So I was wondering if I can rely on it or not in my application?

    @ezio-melotti
    Copy link
    Member

    It looks like an implementation detail to me, so I tend to agree with Georg. I'm not sure if this should be noted in the code though.

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Mar 14, 2013

    Then I think we miss a locale.atod to parse string to Decimal

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 14, 2013

    I agree with "won't fix" for the original issue. These locale functions
    are in effect superseded by PEP-3101 formatting.

    For decimal locale specific formatting, use:

    format(Decimal("1729.1415927"), "n")

    IOW, I don't think new formatting functions should be added to the
    locale module.

    @skrah skrah mannequin closed this as completed Mar 14, 2013
    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Mar 14, 2013

    locale.atof is not about formatting but parsing string into float following the locale.
    For now, the only ways I see to parse a string to get a Decimal is to first convert it into float (which is not good if precision matters) or to use the undocumented parameter.

    @cedk cedk mannequin reopened this Mar 14, 2013
    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Mar 14, 2013

    Cédric Krier <report@bugs.python.org> wrote:

    locale.atof is not about formatting but parsing string into float following the locale.

    You're right. Sorry, I never use these locale functions. My impression is
    that locales are often buggy or differ across platforms (see bpo-16944).

    So actually I now agree that making the parameter official is one reasonable
    solution.

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Mar 15, 2013

    Here is a patch for the documentation.

    @terryjreedy
    Copy link
    Member

    I find the idea of intentionally not documenting a public parameter and the full signature of a function somewhat strange, especially when it is already automatically partially-documented.

    >>> import locale
    >>> help(locale.atof)
    Help on function atof in module locale:
    atof(string, func=<class 'float'>)
        Parses a string as a float according to the locale settings.
    # 2.7, 3.2, 3.3

    Not documenting the full signature of a function seems to me contrary to proper policy. That aside, the func parameter is, to me, a useful feature, not just an implementation detail

    The way to have factored out the common normalization without a func parameter is obvious: define a private normalization function.

    def _anormalize(string):
        "remove thousands separators, make decimal dot"
        ts = localeconv()['thousands_sep']
        if ts:
            string = string.replace(ts, '')
        #next, replace the decimal point with a dot
        dd = localeconv()['decimal_point']
        if dd:
            string = string.replace(dd, '.')
        return string
    
    def atof(string):
        "Parses a string as a float according to the locale settings."
        return float(_anormalize(string))
    
    def atoi(string):  # changed from str
        "Converts a string to an integer according to the locale settings."
        return int(_anormalize(string))

    But Martin von Loewis, the original author did not do this. I would not assume that he "thought that copying 3 lines from atof into atoi was a bad idea." without asking him. Whatever his conscious intention, the func parameter *does* have the advantage of allowing alternate float string to number converters. We now have another one in the stdlib besides decimal.Decimal: fractions.Fractions.

    >>> locale.atof('99,999.99', F)
    Fraction(9999999, 100)
    # versus
    >>> F(locale.atof('99,999.99'))
    Fraction(6871946986405233, 68719476736)

    There are also 3rd party float implementations, such as indefinite precision binary floats. Does anyone still object to properly documenting this useful feature? I am willing to do the commits.

    As to the patch and atof docstring, I thinks 'converts' (used in atoi docstring) is better than 'parses'. So I would change both.

    @ezio-melotti
    Copy link
    Member

    The function was introduced by Guido in f5b55311e79d.

    I think it would have been better if atof had another name (e.g. _atof) and that atof and atoi were implemented as:

    def atof(str):
        return _atof(str, float)
    def atoi(str):
        return _atof(str, int)

    Even better would have been to have _atof return the string without applying any function, and let e.g. atoi return int(_atof(str)). This could have been documented publicly and used to build other functions.
    However it's probably too late for such refactoring.

    @terryjreedy
    Copy link
    Member

    The refactoring could be done if we were willing to give the normalize function a public name, so people could write Decimal(delocalize(<localized float string>)) or if we were willing to add atod and atofr (fraction). However, simply adding a few words to the doc is a lot easier.

    @ezio-melotti
    Copy link
    Member

    It's easier, but we will be exposing an API that is not too elegant IMHO. The refactoring could provide a better API, but OTOH it will make it available for 3.4+ only, and it would break backward compatibility if the old API is removed (even though it's not documented, so in theory we could do that).

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Oct 17, 2014

    So what about this patch?
    It adds a delocalize method while keeping the atof func parameter for backward compatibility.

    @pitrou pitrou added stdlib Python modules in the Lib dir and removed docs Documentation in the Doc dir labels Oct 17, 2014
    @pitrou
    Copy link
    Member

    pitrou commented Oct 17, 2014

    The patch's approach looks reasonable to me.

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Oct 18, 2014

    A new version with unittest.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 18, 2014

    Thanks for the updated patch, Cédric. Just some remarks:

    • the documentation should mention that the return value is still a string
    • the documentation needs a "versionadded" marker next to the new function
    • have you already signed the contributor's agreement? otherwise you should sign it at https://www.python.org/psf/contrib/contrib-form/

    @cedk
    Copy link
    Mannequin Author

    cedk mannequin commented Oct 18, 2014

    Add return value is string in doc
    Add versionadded
    And yes I signed the agreement.

    @pitrou
    Copy link
    Member

    pitrou commented Oct 23, 2014

    Thank you! The patch looks good to me, I'm going to apply it.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 23, 2014

    New changeset aee097e5a2b2 by Antoine Pitrou in branch 'default':
    Issue bpo-13918: Provide a locale.delocalize() function which can remove
    https://hg.python.org/cpython/rev/aee097e5a2b2

    @pitrou
    Copy link
    Member

    pitrou commented Oct 23, 2014

    Done. Thank you for your contribution!

    @pitrou pitrou closed this as completed Oct 23, 2014
    @vstinner
    Copy link
    Member

    + :const:'LC_NUMERIC`settings.

    a space is missing before "settings", no?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 23, 2014

    Ah, right, thank you.

    @birkenfeld
    Copy link
    Member

    And the first quote is wrong.

    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants