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.

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

Run Flynt to convert str.format to f-strings #464

Merged
merged 7 commits into from Sep 9, 2021
Merged

Run Flynt to convert str.format to f-strings #464

merged 7 commits into from Sep 9, 2021

Conversation

FollowTheProcess
Copy link
Collaborator

I didn't file an Issue for this one because this is very much down to personal developer preference, so feel free to reject this if you don't want it I won't be offended at all! :)

Things like this would be very tedious to do by hand but I discovered an automated tool Flynt to convert the older style "{}".format() strings with newer f"{}" style strings and it seems to have worked great!

Since 3.5 is now EOL, it might make sense to convert to the new format!

Ran `flynt`: https://github.com/ikamensh/flynt against nox
to automatically convert `"{}".format` style strings to the
newer `f"{}"` style strings.
Copy link
Collaborator

@cjolowicz cjolowicz left a comment

Choose a reason for hiding this comment

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

+1 on converting the code base to f-strings, thanks!

Some comments:

  • Sometimes expressions in the converted f-string contain a function call, or involve quotes. In these situations, it would be better for readability to extract the expression into a variable beforehand.
  • I'm wondering why some calls of str.format in the nox directory were not converted. Do you have any idea?
  • Can you include noxfile.py and the files in the tests directory in the conversion?
  • As the diff shows, the blacken session insists on inserting the commas in unrelated files, even though it doesn't do so on main. Didn't investigate as it does not matter that much anyway.

@FollowTheProcess
Copy link
Collaborator Author

+1 on converting the code base to f-strings, thanks!

Some comments:

  • Sometimes expressions in the converted f-string contain a function call, or involve quotes. In these situations, it would be better for readability to extract the expression into a variable beforehand.

I'm inclined to agree with you, I was going to do this but figured I'd see if the conversion was something you were interested in first. I can certainly fix this.

  • I'm wondering why some calls of str.format in the nox directory were not converted. Do you have any idea?

I'm not too sure, I ran Flynt against the entire directory. My best guess is if it doesn't know what to do it just leaves them alone? I'll look into this and maybe just fix the remainders manually.

  • Can you include noxfile.py and the files in the tests directory in the conversion?

Yep can definitely do this!

  • As the diff shows, the blacken session insists on inserting the commas in unrelated files, even though it doesn't do so on main. Didn't investigate as it does not matter that much anyway.

The only thing I can think of is I just ran black directly from the command line (although it was the same version as the blacken session so it shouldn't make a difference?) as the lint session defaults to 3.8 which I don't have on my system (I run 3.9)

@FollowTheProcess
Copy link
Collaborator Author

@cjolowicz I've been able to manually change most of the remaining .format calls but I'm slightly confused by this one

I'm not too familiar with this style of formatting so I'm not entirely sure how to port this to f-strings (or if it's possible):

if return_code not in success_codes:
    logger.error(
        "Command {} failed with exit code {}{}".format(
            full_cmd, return_code, ":" if silent else ""
        )
    )

There a few examples that follow the same pattern and a few in the tests that I could not convert e.g inside a parametrised test where replacing with an empty f-string caused a error (cannot have empty placeholder in f-string):

@pytest.mark.parametrize("offline", [False, True], ids="offline={}".format)

Aside from those I think I've got all of them.

@cjolowicz
Copy link
Collaborator

How about this?

if return_code not in success_codes:
    suffix = ":" if silent else ""
    logger.error(f"Command {full_cmd} failed with exit code {return_code}{suffix}")

The pytest example should be left unchanged, because format is not called there (only partially bound).

@FollowTheProcess
Copy link
Collaborator Author

How about this?

if return_code not in success_codes:
    suffix = ":" if silent else ""
    logger.error(f"Command {full_cmd} failed with exit code {return_code}{suffix}")

The pytest example should be left unchanged, because format is not called there (only partially bound).

Of course! The ":" if silent else "" is just a variable like the others 🤦🏻 That's worked perfectly, I'll push up now thanks!

@henryiii
Copy link
Collaborator

Is there no pyupgrade check? That should convert some (maybe not as many?) of these. Thanks, by the way, I heard about Flynt on Python Bytes, but didn't know how to spell it. :)

@FollowTheProcess
Copy link
Collaborator Author

Is there no pyupgrade check? That should convert some (maybe not as many?) of these. Thanks, by the way, I heard about Flynt on Python Bytes, but didn't know how to spell it. :)

Yeah looks like pyupgrade does this too!

@FollowTheProcess
Copy link
Collaborator Author

I could run pyupgrade --py36-plus on this too if we're interested in modernising any remaining older syntax? Don't just want to lump it on this PR without checking first as it might clutter the narrative?

@theacodes
Copy link
Collaborator

I'm good with this PR as-is, @FollowTheProcess & @cjolowicz. Either of y'all can merge it if you'd like.

@FollowTheProcess FollowTheProcess merged commit 9c696e2 into wntrblm:main Sep 9, 2021
@FollowTheProcess
Copy link
Collaborator Author

Thanks guys! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants