-
-
Notifications
You must be signed in to change notification settings - Fork 60
Add fake f-strings to blurb. Works with 3.5! #288
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
Conversation
Mariatta's review was requested, but she's out of open source for the rest of September 2018. Perhaps request a review from someone else, or wait until October. |
How come travis CI is not run here? |
I don't know anything about it! |
I told @larryhastings that you get ride of Python 3.5 support if you want. I'm too lazy to check which operating systems don't provide Python 3.6 or newer. We can revert Python 3.5 support if we can too many complains :-) |
Yes, but I felt like rising to the challenge! This supports something very similar to f-strings, but still runs under 3.5. (I tested that!) |
I remind both of you that reviewing this PR would get you on the leaderboard for the dev sprints ;-) |
I don't know... this PR is quite evil literally. I just want more people to install Python 3.6 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little nit that doesn't need to be addressed (but I need to prove I read everything!). Looks good to me.
# else: | ||
# print("NOT FIXING LINE {}: {!r}".format(line_number, line)) | ||
# print(f("NOT FIXING LINE {line_number}: {line!r}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I admire your tenacity for even fixing the comments!
@@ -1601,10 +1628,10 @@ def main(): | |||
def handle_option(s, dict): | |||
name = dict.get(s, None) | |||
if not name: | |||
sys.exit('blurb: Unknown option for {}: "{}"'.format(subcommand, s)) | |||
sys.exit(f('blurb: Unknown option for {subcommand}: "{s}"')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably use {s!r}
here instead of "{s}"
, but I realize it's outside the scope of your direct changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would change the name of the variable from s to str?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I would use !r
to add the quotes, instead of adding them manually. I realize it might change the type of quotes from double to single, based on what s
contains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Sorry. Tiny font made s!r
look like str
on my screen.
To address @vstinner 's comment: I think this is a reasonable change as an interim to using f-strings in the future. Once this is done, actually switching to f-strings is trivial. |
Just going to close and re-open the PR, hoping to re-trigger Travis CI. |
No description provided.