Skip to content

/ cpython

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

bpo-38536: Removes trailing space in formatted currency #16864

Merged
merged 3 commits into from Jan 20, 2020
Merged

bpo-38536: Removes trailing space in formatted currency #16864

merged 3 commits into from Jan 20, 2020
+5 −2

Conversation

@ghost
Copy link

ghost commented Oct 20, 2019 •
edited by bedevere-bot

with international=True and symbol following value.

Validated that Ubuntu was affected
and Windows 10 was not affected.
Fix considers both cases.

https://bugs.python.org/issue38536
@the-knights-who-say-ni

This comment has been minimized.

Sign in to view
Copy link

the-knights-who-say-ni commented Oct 20, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@Sanjo, @harsh04081997

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!
@eamanu
eamanu reviewed Oct 20, 2019
View changes
Lib/locale.py
@@ -279,6 +279,8 @@ def currency(val, symbol=True, grouping=False, international=False):
if precedes:
s = smb + (separated and ' ' or '') + s
else:
if international and smb[-1] == ' ':
smb = smb[:-1]

This comment has been minimized.

Sign in to view
Copy link
@eamanu

eamanu Oct 20, 2019

Contributor

why don't use rstrip()?

This comment has been minimized.

Sign in to view
Copy link
@ghost

ghost Oct 21, 2019

Author

To be more specific about the case (1 trailing space).
@eamanu

This comment has been minimized.

Sign in to view
Copy link
Contributor

eamanu commented Oct 20, 2019

@Sanjo Don't forget sign cla
@ghost

This comment has been minimized.

Sign in to view
Copy link
Author

ghost commented Oct 21, 2019

CLA has been signed and https://check-python-cla.herokuapp.com/ also gave back success.
@Mariatta
Mariatta approved these changes Oct 21, 2019
View changes
@Mariatta

This comment has been minimized.

Sign in to view
Copy link
Member

Mariatta commented Oct 21, 2019

Thanks for fixing this bug. Can you please write the news entry.
@ghost

This comment has been minimized.

Sign in to view
Copy link
Author

ghost commented Oct 21, 2019

Can you please write the news entry.

@Mariatta Done (with 17867bd).
@eamanu
eamanu approved these changes Oct 22, 2019
View changes
Copy link
Contributor

eamanu left a comment

I don't know if write the example is a good improve. but LGTM :-)
@ghost

This comment has been minimized.

Sign in to view
Copy link
Author

ghost commented Oct 31, 2019

Just FYI, the CLA has been signed by me. The bot seems to have some quirk with removing the label. Let me know if there is anything else necessary from my side for this pull request to be merged.
@Mariatta

This comment has been minimized.

Sign in to view
Copy link
Member

Mariatta commented Nov 1, 2019

We check CLA record for everyone who made commit to the PR, not just the person who opened the PR. The code seems to be committed by user "A A", and perhaps "A A" has not signed CLA.

Either have A A sign the CLA (and create bpo account) or modify this PR so it only contains your own commit.
Jonas Aschenbrenner and others added 2 commits Oct 20, 2019
Jonas Aschenbrenner
bpo-38536: Removes trailing space in formatted currency with internat…
e99bcc5 
…ional=True and symbol following value

Validated that Ubuntu was affected
and Windows 10 was not affected.
Fix considers both cases.
@blurb-it Jonas Aschenbrenner
📜🤖 Added by blurb_it.
Loading status checks…
7f3525e
@ghost

This comment has been minimized.

Sign in to view
Copy link
Author

ghost commented Nov 1, 2019

@Mariatta I have fixed the commit to contain the proper author and committer. There was just some configuration for Git missing on my newly setup computer. Best regards.
@methane

This comment has been minimized.

Sign in to view
Copy link
Member

methane commented Nov 25, 2019

Validated that Ubuntu was affected
and Windows 10 was not affected.

Have you tried all locale on Ubuntu and Windows?
What is output of the Windows 10?
@csabella

This comment has been minimized.

Sign in to view
Copy link
Contributor

csabella commented Jan 17, 2020

@Sanjo, please address @methane's questions. Thank you!
@ghost

This comment has been minimized.

Sign in to view
Copy link
Author

ghost commented Jan 17, 2020

Have you tried all locale on Ubuntu and Windows?

No, I have not tried the all locale on Ubuntu and Windows.

What is output of the Windows 10?

The output on Windows 10 has no trailing space.
@methane
methane reviewed Jan 18, 2020
View changes
Misc/NEWS.d/next/Library/2019-10-21-20-24-51.bpo-38536.beZ0Sk.rst Outdated Show resolved Hide resolved
Jonas Aschenbrenner @methane
Update Misc/NEWS.d/next/Library/2019-10-21-20-24-51.bpo-38536.beZ0Sk.rst
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
ffcc468 
Co-Authored-By: Inada Naoki <songofacandy@gmail.com>
@methane methane merged commit e96d954 into python:master Jan 20, 2020
9 checks passed
9 checks passed
Docs
Details
Windows (x86)
Details
Windows (x64)
Details
macOS
Details
Ubuntu
Details
Azure Pipelines PR #20200118.31 succeeded
Details
bedevere/issue-number Issue number 38536 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA signed
Projects
None yet
Milestone
No milestone
6 participants
@the-knights-who-say-ni @eamanu @Mariatta @methane @csabella @bedevere-bot
You can’t perform that action at this time.