Skip to content

Conversation

@berkerpeksag
Copy link
Member

@berkerpeksag berkerpeksag commented Aug 3, 2018

Copy link
Member

@ericvsmith ericvsmith left a comment

Choose a reason for hiding this comment

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

LGTM

@berkerpeksag
Copy link
Member Author

@ericvsmith thanks for the review! Do you have any suggestions on making the NEWS entry more descriptive? I'm pretty sure that I did a bad job explaining the bug :)

@ericvsmith
Copy link
Member

I'm not sure it's much of an improvement, but had I been writing it from scratch I would have gone with:

Fix %-formatting in :meth:pathlib.PurePath.with_suffix when formatting an error message.

Although if I'd been doing it, I would have changed it to an f-string (of course!). I didn't suggest that just to minimize churn. But seriously: one of the motivating points of str.format (and later f-strings) was to get rid of exactly this kind of error. Maybe commit this fix (or not), then go through the entire file and use f-strings? There are a number of other error messages that seem to have this same problem, such as in both gethomedir methods. Not sure about others: the analysis is harder than just changing to f-strings. I realize this would make pre-3.6 backports harder, but that shouldn't still be an issue (barring finding a security-related problem).

Anyway, take that last suggestion or not, you won't hurt my feelings.

@berkerpeksag
Copy link
Member Author

Thanks for the suggestions!

Initially, I thought about using f-strings in the whole module, but then I didn't want to introduce code churn and come up with artificial examples like the ones at https://bugs.python.org/issue27161 I'm planning to triage/review/merge pathlib issues and PRs in the coming weeks, so I can write proof-of-concept patch to implement your suggestion.

@berkerpeksag berkerpeksag merged commit 423d05f into python:master Aug 11, 2018
@miss-islington
Copy link
Contributor

Thanks @berkerpeksag for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@berkerpeksag berkerpeksag deleted the pathlib-with-suffix branch August 11, 2018 05:45
@bedevere-bot
Copy link

GH-8731 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 11, 2018
(cherry picked from commit 423d05f)

Co-authored-by: Berker Peksag <berker.peksag@gmail.com>
berkerpeksag added a commit that referenced this pull request Aug 11, 2018
(cherry picked from commit 423d05f)

Co-authored-by: Berker Peksag <berker.peksag@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants