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.

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

Updated string formatting #7614

Merged
merged 1 commit into from
Jan 25, 2020
Merged

Updated string formatting #7614

merged 1 commit into from
Jan 25, 2020

Conversation

deepak1725
Copy link
Contributor

Updated string formatting
#6973

@pfmoore
Copy link
Member

pfmoore commented Jan 20, 2020

We don't normally update vendored packages, so could you remove the changes to files under src/pip/_vendor?

@xavfernandez xavfernandez added the skip news Does not need a NEWS file entry (eg: trivial changes) label Jan 20, 2020
@xavfernandez
Copy link
Member

And no need for a newsfile.

pradyunsg
pradyunsg reviewed Jan 20, 2020
View reviewed changes
@deepak1725 deepak1725 requested a review from pradyunsg January 21, 2020 05:19
@deepak1725
Copy link
Contributor Author

@pradyunsg Any updates on this..?

pradyunsg
pradyunsg reviewed Jan 23, 2020
View reviewed changes
@deepak1725 deepak1725 requested a review from pradyunsg January 23, 2020 13:14
@deepak1725
Copy link
Contributor Author

@pradyunsg Any more nits..?

pradyunsg
pradyunsg approved these changes Jan 25, 2020
View reviewed changes
Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

There's still a few areas where I'd personally use a different code style but, adopting black should fix those. :)

LGTM.

@deepak1725
Copy link
Contributor Author

deepak1725 commented Jan 25, 2020 •
edited
Loading

@pradyunsg Sure, Thanks.
I can see a few more instances of %s type formatting, will be raising another PR soon.
Can you merge this..?

@chrahunt chrahunt merged commit 153e22a into pypa:master Jan 25, 2020
@chrahunt
Copy link
Member

Thank you

@uranusjr
Copy link
Member

I saw a few instances where logging %s arguments got converted into .format. I don’t think this is a good idea.

@deepak1725 deepak1725 mentioned this pull request Jan 29, 2020
Merged
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Feb 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@chrahunt chrahunt chrahunt approved these changes

@pradyunsg pradyunsg pradyunsg approved these changes

Assignees
No one assigned
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@deepak1725 @pfmoore @xavfernandez @chrahunt @uranusjr @pradyunsg