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

Switch all string addition to using f-strings #1774

Merged
merged 1 commit into from Mar 3, 2021

Conversation

s-t-e-v-e-n-k
Copy link
Collaborator

Now that we no longer support Python 3.5, move to the best feature 3.6+
gives us, f-strings! This will likely be a bit of a moving target if
older code gets merged.

@sfdye
Copy link
Member

sfdye commented Dec 7, 2020

Let's use pre-commit to enforce this

@s-t-e-v-e-n-k
Copy link
Collaborator Author

@sfdye Good poiint, I'll add that in this PR, and then merge it when CI is happy again.

@s-t-e-v-e-n-k
Copy link
Collaborator Author

Wait a second, how does pyupgrade enforce this?

@sfdye
Copy link
Member

sfdye commented Dec 8, 2020

if you use the --python36 option, it will enforce f-string i think

@henryiii
Copy link

henryiii commented Dec 9, 2020

It doesn't enforce f-strings everywhere, just where it thinks they are better. ref. It would be good to use f-strings as much as possible, because they are faster. :)

@sfdye
Copy link
Member

sfdye commented Dec 10, 2020

It doesn't enforce f-strings everywhere, just where it thinks they are better. ref. It would be good to use f-strings as much as possible, because they are faster. :)

That's the more accurate way to put it 😄

@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #1774 (aa72d49) into master (f299699) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1774   +/-   ##
=======================================
  Coverage   98.76%   98.76%           
=======================================
  Files          51       51           
  Lines        2677     2678    +1     
=======================================
+ Hits         2644     2645    +1     
  Misses         33       33           
Impacted Files Coverage Δ
github/MainClass.py 95.62% <100.00%> (ø)
github/ProjectCard.py 100.00% <100.00%> (ø)
github/Requester.py 97.47% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f299699...aa72d49. Read the comment docs.

@s-t-e-v-e-n-k
Copy link
Collaborator Author

@sfdye pyupgrade did some far-reaching changes, but shall we merge this and cut a .1 release?

@sfdye
Copy link
Member

sfdye commented Dec 14, 2020

The changes are not just f-string, more like applying pyupgrade on whole codebase. Could you update the PR title/description and add the commit sha our ignore list 😄

Comment on lines 56 to 61
for element in self.__elements:
yield element
yield from self.__elements
while self._couldGrow():
newElements = self._grow()
for element in newElements:
yield element
Copy link
Member

Choose a reason for hiding this comment

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

sweet

@s-t-e-v-e-n-k
Copy link
Collaborator Author

So the problem is, if we disentangle this, we get a bunch of changes that pyuppgrade makes calling .format(). I can try again if you wish.

@sfdye
Copy link
Member

sfdye commented Dec 14, 2020

So the problem is, if we disentangle this, we get a bunch of changes that pyuppgrade makes calling .format(). I can try again if you wish.

What is the problem? I don't understand

@s-t-e-v-e-n-k
Copy link
Collaborator Author

What is the problem? I don't understand

Objection removed -- I'll push up a new PR with pyupgrade. Given the formative changes that it will introduce, like yield form, I don't think we should add it to .git-blame-ignore-revs

@sfdye
Copy link
Member

sfdye commented Dec 14, 2020

I feel most changes (remove unicode literal, removing shebang/convert to f-string) are inconsequential

Now that we no longer support Python 3.5, stop using string addition
everywhere it makes sense, and move to the best feature 3.6+ gives us,
f-strings!
@s-t-e-v-e-n-k s-t-e-v-e-n-k merged commit 290b627 into PyGithub:master Mar 3, 2021
@s-t-e-v-e-n-k s-t-e-v-e-n-k deleted the yay-f-strings branch October 21, 2021 04:02
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.

None yet

5 participants