Skip to content

STY: Old formatting to f-strings (PEP-498) #919

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
Merged
merged 14 commits into from
Jul 6, 2020
Merged

STY: Old formatting to f-strings (PEP-498) #919

merged 14 commits into from
Jul 6, 2020

Conversation

jond01
Copy link
Contributor

@jond01 jond01 commented May 27, 2020

%, '{}'.format -> f'{}'
F-strings are supported from py3.6 which is nibabel's minimal python version (up-to-date).

@jond01
STY: Old formatting to f-strings (PEP-498)
b10fbc1 
%, '{}'.format -> f'{}'
F-strings are supported from py3.6 which is nibabel's
minimal python version (up-to-date).

Was done with:
https://github.com/ikamensh/flynt

Excluding versioneer.py

First commit changes were reviewed manually
@jond01 jond01 marked this pull request as draft May 27, 2020 18:30
@jond01 jond01 mentioned this pull request May 27, 2020
Closed
5 tasks
@codecov
Copy link

codecov bot commented May 27, 2020 •
edited
Loading

Codecov Report

Merging #919 into master will increase coverage by 0.12%.
The diff coverage is 78.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
+ Coverage   91.73%   91.85%   +0.12%     
==========================================
  Files          97       96       -1     
  Lines       12357    12344      -13     
  Branches     2177     2176       -1     
==========================================
+ Hits        11336    11339       +3     
+ Misses        684      674      -10     
+ Partials      337      331       -6
Impacted Files Coverage Δ
nibabel/cmdline/dicomfs.py 18.96% <0.00%> (ø)
nibabel/cmdline/diff.py 93.70% <ø> (ø)
nibabel/filebasedimages.py 85.80% <0.00%> (ø)
nibabel/filename_parser.py 92.04% <ø> (ø)
nibabel/fileslice.py 97.26% <0.00%> (ø)
nibabel/keywordonly.py 100.00% <ø> (ø)
nibabel/openers.py 95.09% <0.00%> (+0.04%) ⬆️
nibabel/spatialimages.py 96.44% <ø> (ø)
nibabel/cmdline/utils.py 80.64% <25.00%> (ø)
nibabel/casting.py 86.14% <33.33%> (+0.86%) ⬆️
... and 53 more

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 b48fb17...0dd06ee. Read the comment docs.

jond01 added 3 commits June 4, 2020 23:59
@jond01
More f-strings
b183b73 
flynt -ll 999 .
All changes were reviewed manually
@jond01
More f-strings
2271c97 
flynt -ll 999 .
All changes were reviewed manually
(Excluding versioneer)
@jond01
A bit more f-strings, but that's it
6b15a97
@jond01 jond01 marked this pull request as ready for review June 18, 2020 18:03
@jond01 jond01 changed the title WIP, STY: Old formatting to f-strings (PEP-498) STY: Old formatting to f-strings (PEP-498) Jun 18, 2020
@jond01
Limit lines to 100 columns
fdfe1cd 
Excluding URLs which cannot be split and test code
effigies
effigies reviewed Jun 22, 2020
View reviewed changes
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Thanks for this! I haven't finished reading through (~30 files to go), but I've made a number of comments that you might find useful before I have time to finish.

Feel free to push back on things that seem unreasonable, by the way.

One additional thing: Let's not touch externals/ or _version.py. externals are intended to be exact copies of an externally sourced module, except when they produce warnings or errors, and this isn't a necessary change. Similarly, _version.py is generated by versioneer, and any changes we make can be potentially reverted, so switching it to f-strings means more things to pay attention to if it ever needs to be regenerated.

jond01
jond01 commented Jun 22, 2020
View reviewed changes
jond01
jond01 commented Jun 22, 2020
View reviewed changes
jond01
jond01 commented Jun 22, 2020
View reviewed changes
jond01
jond01 commented Jun 22, 2020
View reviewed changes
jond01
jond01 commented Jun 22, 2020
View reviewed changes
jond01
jond01 commented Jun 22, 2020
View reviewed changes
Copy link
Contributor Author

@jond01 jond01 left a comment

Choose a reason for hiding this comment

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

Reviewed again, before accepting all suggestions and adding mine

jond01 and others added 2 commits June 22, 2020 21:53
@jond01 @effigies
STY: Apply suggestions from code review
092dc9b 
Co-authored-by: Chris Markiewicz <[email protected]>
@jond01
STY: Use f-strings only where necessary
a3f51aa 
+ remove str calls in f-strings, one line if possible
@jond01
Copy link
Contributor Author

jond01 commented Jun 22, 2020

I cannot exactly understand CI failure:
https://travis-ci.com/github/nipy/nibabel/jobs/352352424

@effigies
Copy link
Member

Travis sometimes glitches. I've restarted the job. We'll see if it repeats.

effigies
effigies reviewed Jul 6, 2020
View reviewed changes
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Thanks for your patience. I've had a chance to look through it all now. Just some small nitpicks.

jond01 and others added 2 commits July 6, 2020 10:03
jond01
jond01 commented Jul 6, 2020
View reviewed changes
Copy link
Contributor Author

@jond01 jond01 left a comment

Choose a reason for hiding this comment

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

Reviewed comments

@effigies
Copy link
Member

effigies commented Jul 6, 2020

Thanks!

@effigies effigies merged commit 0ba55d4 into nipy:master Jul 6, 2020
@jond01 jond01 deleted the fstrings branch July 6, 2020 12:57
@effigies effigies added this to the 3.2.0 milestone Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@effigies effigies effigies left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@jond01 @effigies