Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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

Conversation

ghost
Copy link

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

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
Copy link

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 reviewed 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]
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't use rstrip()?

Copy link
Author

Choose a reason for hiding this comment

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

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

@eamanu
Copy link
Contributor

eamanu commented Oct 20, 2019

@sanjo Don't forget sign cla

@ghost
Copy link
Author

ghost commented Oct 21, 2019

CLA has been signed and https://check-python-cla.herokuapp.com/ also gave back success.

@Mariatta
Copy link
Member

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

@ghost
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 reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

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

@ghost
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
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.

SanjoSolutions and others added 2 commits November 1, 2019 15:23
@SanjoSolutions
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 @SanjoSolutions
📜🤖 Added by blurb_it.
7f3525e
@ghost
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
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
Copy link
Contributor

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

@ghost
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 reviewed changes
@methane methane merged commit e96d954 into python:master Jan 20, 2020
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@methane @shihai1991
bpo-38536: locale: Remove trailing space in formatted currency (pytho…
91a0674 
…nGH-16864)
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@the-knights-who-say-ni @eamanu @Mariatta @methane @csabella @bedevere-bot @SanjoSolutions @sanjo