bpo-10379: deprecate locale.format in lieu of locale.format_string #259

plusminushalf · 2017-02-23T19:29:22Z

>>> import locale; >>> print(locale.format('%.0f KB', 100)) 100 KB >>> locale.format.__doc__ 'Deprecated, use format_string instead.'

the-knights-who-say-ni · 2017-02-23T19:29:23Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:

Sign the PSF contributor agreement
Wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

bitdancer · 2017-02-23T19:36:55Z

Thanks for working on this. Doc changes are also needed.

bitdancer · 2017-02-23T19:37:24Z

As well as tests for the deprecation message.

bitdancer · 2017-02-24T15:32:29Z

Somehow I hadn't noticed that 'monetary' wasn't supported by format_string. So that is actually an enhancement, in addition to the deprecation, and needs to be documented as such (versionchanged, whats new entry). Also, format itself shouldn't be changed, just deprecated (that is, the deprecation message code added, but nothing else in it changed).

plusminushalf · 2017-02-24T17:40:25Z

@bitdancer I don't think to keep locale.format as it is and just displaying deprecated warning would be of any use. I think it is safe to call locale.format_string internally.

bitdancer · 2017-02-24T18:14:51Z

Doc/library/locale.rst

+ grouping strings.
+ Replaces :meth:`format`.
+



The description of the monetary parameter should be added to the regular docs, and the version changed phrase should just say "the monetary keyword parameter was added"

bitdancer · 2017-02-24T18:18:45Z

That's the way we normally do deprecations: leave the existing code in place (so its behavior doesn't change) but point people to the preferred solution. It would be acceptable if there were no behavior changes, but the changed tests prove that there are.

bitdancer

Thanks. Almost there :)

It would also be good to have an entry for the What's New document for 3.7 for the addition of the monetary parameter to format_string, as well as the deprecation of format. I haven't looked; if the 3.7 what's new hasn't been organized yet, just stick in a placeholder sentence with a reference to the bpo issue number.

bitdancer · 2017-02-24T20:16:25Z

Doc/library/locale.rst

 .. versionchanged:: 3.7
- Added *monetary*, if true the conversion uses monetary thousands separator and
- grouping strings.
+ The *monetary* keyword parameter was added.


We still need the description of the monetary keyword (presumably copy and pasted from the existing format entry) in the description of the format_string function.

In fact, what we really want to do is copy most of the 'format' description into format_string, since it seems wrong somehow to refer to the docs of a deprecated from from the preferred function.

bitdancer · 2017-02-24T20:20:23Z

Lib/test/test_locale.py

 self._test_format("%-10.f", 4200, grouping=0, out='4200'.ljust(10))

+ def test_format_deprecation(self):
+ with warnings.catch_warnings(record=True) as w:


You can use assertWarns here instead.

bitdancer · 2017-02-24T20:24:47Z

Doc/library/locale.rst


+ .. versionchanged:: 3.7
+ The *monetary* keyword parameter was added.
+ Replaces :meth:`format`.


I don't think the 'replaces' sentence is needed. The deprecated on format covers that.

Fixed recommended CR changes

bitdancer

Thanks for sticking with this :)

bitdancer · 2017-02-26T17:19:39Z

Doc/library/locale.rst

 locale settings into account.

+ .. versionchanged:: 3.7
+ The *monetary* keyword parameter was added.


Missing indentation.

bitdancer · 2017-02-26T17:23:52Z

Doc/whatsnew/3.7.rst

 function instead.

+* Deprecated :meth:`format` from :mod:`locale`, use the :meth:`format_string`
+ instead. Added another argument *monetary* in :meth:`format_string` of


format hasn't been removed, so the deprecation should go in the deprecation section. The feature addition of the monetary keyword should go in the 'improved modules' section.

codecov · 2017-02-27T13:56:06Z

Codecov Report

Merging #259 into master will decrease coverage by -1.01%.
The diff coverage is 71.42%.

@@ Coverage Diff @@ ## master #259 +/- ## ========================================== - Coverage 83.38% 82.38% -1.01%  ========================================== Files 1367 1428 +61 Lines 344897 351221 +6324 ========================================== + Hits 287596 289342 +1746  - Misses 57301 61879 +4578

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae160bb...6097e47. Read the comment docs.

bitdancer · 2017-02-27T16:56:10Z

Doc/whatsnew/3.7.rst

 Improved Modules
 ================

+* Added another argument *monetary* in :meth:`format_string` of :mod:`locale`.


There should be a subsection header before this line for the locale module (the modules with enhancements are listed in this section in alphabetical order).

plusminushalf · 2017-03-03T15:31:22Z

@bitdancer is it good to go now?

bitdancer · 2017-03-03T17:03:25Z

Yes, I think so. I'd like to do a final overall review before I approve it, though. With any luck I'll do that tonight.

plusminushalf · 2017-03-03T17:04:48Z

Thanks! :)

plusminushalf · 2017-03-17T15:31:03Z

Hey, @bitdancer have you reviewed this?

bitdancer · 2017-03-17T16:39:59Z

I did...and I feel bad that I have some more changes to suggest, so I was going to make them myself and submit it to the PR, but I haven't actually figured out how to do that yet.

plusminushalf · 2017-03-17T16:44:36Z

@bitdancer well I think you can suggest me the changes I'll do my best to figure them out.

bitdancer · 2017-03-17T17:48:39Z

Doc/library/locale.rst

 For whole format strings, use :func:`format_string`.

+ .. deprecated:: 3.7
+ Use :meth:`format_string` instead


So looking at this again, I think what we should do is move 'format' under 'format_string', and change its body to say "works like format_string but only accepts a single format specification. That followed by the deprecation notice will make it a lot less likely anyone will use it :)

bitdancer · 2017-03-17T17:49:43Z

Doc/whatsnew/3.7.rst

+If *monetary* is true, the conversion uses monetary thousands separator and
+grouping strings.
+(Contributed by Garvit in :issue:`10379`.)
+


Small nit: our convention appears to be to put the Contributed by as part of the paragraph when there's only one paragraph. Ends up formatted the same in html, but might as well make the ReST consistent as well.

bitdancer · 2017-03-17T17:50:19Z

Lib/locale.py

+ """Deprecated, use format_string instead."""
+ warnings.warn(
+ "This method will be removed in future versions. "
+ "Use 'locale.format_string()' instead.",


I think this should read "This method will be removed in a future version of Python."

bitdancer · 2017-03-17T17:51:32Z

Lib/locale.py

 """Formats a string in the same way that the % formatting would use,
 but takes the current locale into account.
- Grouping is applied if the third parameter is true."""
+ Grouping is applied if the third parameter is true.


As long as we are editing this, I think there should be a blank line after the first sentence, before the 'Grouping' sentence.

bitdancer · 2017-03-17T17:54:15Z

Lib/locale.py

 "the 'C' locale.")

- s = format('%%.%if' % digits, abs(val), grouping, monetary=True)
+ s = format_string('%%.%if' % digits, abs(val), grouping, monetary=True)


Since we know there's only one format string here, I think this should be a call to _format.

bitdancer · 2017-03-17T17:55:08Z

Lib/locale.py

 def str(val):
 """Convert float to string, taking the locale into account."""
- return format("%.12g", val)
+ return format_string("%.12g", val)


As above, I think this should be _format.

Refactoring Doc, moving format doc below format_string, this is to make sure people have lesser probability of using format in future. Also using _format method inside locale module, since we know there is only one format function that is _format

bitdancer · 2017-03-20T17:01:12Z

There was no need to move format in the source file, but it doesn't hurt, either.

plusminushalf · 2017-03-27T02:35:31Z

Hey @bitdancer is there anything left to get this merged?

bitdancer · 2017-03-27T13:46:09Z

Just adding the Misc/NEWS entry. I'm not sure what status is on how to do that currently, but I think it is still manual edit of that file. I haven't had time yet to learn how to update a PR before merge, so if you add the entry, I'll hit merge :)

bitdancer · 2017-03-28T14:09:37Z

Misc/NEWS

 - bpo-29534: Fixed different behaviour of Decimal.from_float()
 for _decimal and _pydecimal. Thanks Andrew Nester.

+- bpo-10379: Deprecate locale.format in lue of locale.format_string


Google says it's spelled 'lieu'. Also the entry should note that the monetary argument was added to format_string. So, how about "locale.format_string now supports the 'monetary' keyword argument, and locale.format is deprecated."

Update the Stackless patches for readthedocs.org based on the documentation. This simplified configuration is still untested.

Well, the previous commit didn't build on readthedocs.org. - Rename .readthedocs.yml to .readthedocs.yaml See https://docs.readthedocs.io/en/stable/config-file/index.html - Add a requirements file Doc/slp_readthedocs_requirements.txt and require the same packages as specified in Makefile. Already set the Sphinx version to 2.0.1, because upstream commit 7d23dbe

Set the css-file to "pydoctheme.css".

Remove the explicit specification of the css-file for readthedocs.org. It is not needed any more.

bpo-10379: deprecate locale.format in lue of locale.format_string

d0f9863

the-knights-who-say-ni added the CLA not signed label Feb 23, 2017

Mariatta removed the CLA not signed label Feb 24, 2017

the-knights-who-say-ni added the CLA signed label Feb 24, 2017

Adding tests and documentation changes

820e289

Adding version changed info for locale.format

4cfd737

bitdancer requested changes Feb 24, 2017

View reviewed changes

Deprecating but not changing the function defination

a08419f

bitdancer requested changes Feb 24, 2017

View reviewed changes

Adding documentation inside what's new in 3.7

4e24a3f

Fixed recommended CR changes

plusminushalf force-pushed the fix-bpo-10379 branch from 052f6bc to 4e24a3f Compare February 25, 2017 15:07

bitdancer requested changes Feb 26, 2017

View reviewed changes

Adding whatsnew documentation in proper segments

6097e47

plusminushalf added 2 commits February 27, 2017 19:30

Merge branch 'master' of github.com:python/cpython into fix-bpo-10379

04577c2

fixing documentation bug

2039406

bitdancer requested changes Feb 27, 2017

View reviewed changes

Adding locale subsection

5b100b8

Merge branch 'master' of github.com:python/cpython into fix-bpo-10379

041349f

bitdancer reviewed Mar 17, 2017

View reviewed changes

bitdancer approved these changes Mar 20, 2017

View reviewed changes

Adding entry to Misc/NEWS about changes

8190fa9

bitdancer requested changes Mar 28, 2017

View reviewed changes

plusminushalf changed the title ~~bpo-10379: deprecate locale.format in lue of locale.format_string~~ bpo-10379: deprecate locale.format in lieu of locale.format_string Mar 28, 2017

CR fixes

7e0bfcf

bitdancer approved these changes Mar 28, 2017

View reviewed changes

bitdancer merged commit 1cf93a7 into python:master Mar 28, 2017

akruis added a commit to akruis/cpython that referenced this pull request Aug 8, 2021

Stackless issue python#259: Update Doc/conf.py and .readthedocs.yml

b87f882

Update the Stackless patches for readthedocs.org based on the documentation. This simplified configuration is still untested.

akruis added a commit to akruis/cpython that referenced this pull request Aug 8, 2021

Stackless issue python#259: Update Doc/conf.py

53ca885

Set the css-file to "pydoctheme.css".

akruis added a commit to akruis/cpython that referenced this pull request Aug 8, 2021

Stackless issue python#259: Update Doc/conf.py

e61be11

Remove the explicit specification of the css-file for readthedocs.org. It is not needed any more.

roe-dl mentioned this pull request Oct 27, 2024

AttributeError: module 'locale' has no attribute 'format'. Did you mean: '_format'? (Python 3.12) poblabs/weewx-belchertown#954

Closed

Uh oh!

bpo-10379: deprecate locale.format in lieu of locale.format_string #259

bpo-10379: deprecate locale.format in lieu of locale.format_string #259

Uh oh!

Conversation

plusminushalf commented Feb 23, 2017

the-knights-who-say-ni commented Feb 23, 2017

bitdancer commented Feb 23, 2017

bitdancer commented Feb 23, 2017

bitdancer commented Feb 24, 2017

plusminushalf commented Feb 24, 2017

Choose a reason for hiding this comment

bitdancer commented Feb 24, 2017

bitdancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitdancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 27, 2017

Codecov Report

Choose a reason for hiding this comment

plusminushalf commented Mar 3, 2017

bitdancer commented Mar 3, 2017

plusminushalf commented Mar 3, 2017

plusminushalf commented Mar 17, 2017

bitdancer commented Mar 17, 2017

plusminushalf commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitdancer commented Mar 20, 2017

plusminushalf commented Mar 27, 2017

bitdancer commented Mar 27, 2017

Choose a reason for hiding this comment

Labels

4 participants