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.

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

Add Consider-using-f-string checker #4796

Merged
merged 7 commits into from Aug 30, 2021
Merged

Add Consider-using-f-string checker #4796

merged 7 commits into from Aug 30, 2021

Conversation

DanielNoord
Copy link
Collaborator

@DanielNoord DanielNoord commented Aug 3, 2021 •
edited

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature, or an important bug fix, add a What's New entry in
    doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Type of Changes

Type
✓ ✨ New feature

Description

Add consider-using-f-string checker
This adds a checker for normal strings which are formatted
with .format() or '%'.
The message is a convention to nudge users towards using f-strings.
This closes #3592

@DanielNoord DanielNoord marked this pull request as draft August 3, 2021 22:11
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Aug 4, 2021
@DanielNoord DanielNoord force-pushed the f-string-convention branch 2 times, most recently from 7fe2a32 to 9e64e5f Compare August 19, 2021 21:26
@DanielNoord
Add consider-using-f-string checker
7d81b99 
This adds a checker for normal strings which are formatted
with ``.format()`` or '%'.
The message is a convention to nudge users towards using f-strings.
This closes pylint-dev#3592
@DanielNoord
Update pylint code to use f-strings
8bc9668 
After adding `consider-using-f-strings` the codebase showed numerous
cases of formatting which could be f-strings.
This commit changes most of these to become f-strings, or adds ignores.
@coveralls
Copy link

coveralls commented Aug 20, 2021 •
edited

Pull Request Test Coverage Report for Build 1176970109

  • 114 of 146 (78.08%) changed or added relevant lines in 43 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 92.746%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pylint/checkers/similar.py 1 2 50.0%
pylint/checkers/spelling.py 1 2 50.0%
pylint/checkers/strings.py 34 35 97.14%
pylint/config/option.py 0 1 0.0%
pylint/lint/run.py 0 1 0.0%
pylint/pyreverse/inspector.py 1 2 50.0%
pylint/pyreverse/utils.py 0 1 0.0%
pylint/reporters/base_reporter.py 0 1 0.0%
pylint/reporters/text.py 1 2 50.0%
pylint/reporters/ureports/nodes.py 1 2 50.0%
Files with Coverage Reduction New Missed Lines %
pylint/config/man_help_formatter.py 1 32.26%
Totals Coverage Status
Change from base Build 1169727876: 0.01%
Covered Lines: 13488
Relevant Lines: 14543
💛 - Coveralls

@DanielNoord DanielNoord marked this pull request as ready for review August 20, 2021 11:18
@DanielNoord
Copy link
Collaborator Author

Many changes in the second commit of this PR. However, I think they are warranted as f-strings are considerably faster, see here for example.
Let me know if anything needs to be changed!

Pierre-Sassoulas
Pierre-Sassoulas requested changes Aug 20, 2021
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Huge cleanup work alongside a very useful new checker, thanks a lot ! Did you check the change in performance by using the benchmark result locally ? If f-string are faster if could be another good news 😄 I have minor comments, let me know what you think.

pylint/checkers/strings.py Outdated Show resolved Hide resolved
tests/functional/l/logging_not_lazy.py
@@ -14,7 +14,6 @@
renamed_logging.warn('%s' + ' the rest of a single string') # [logging-not-lazy]
renamed_logging.log(renamed_logging.INFO, var_name + var) # [logging-not-lazy]

var_name = 'Var:'
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this line ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't think it added anything after line 8. Seems a duplicate

tests/functional/r/raise/raising_format_tuple.py Outdated Show resolved Hide resolved
@DanielNoord
Copy link
Collaborator Author

Should be good to go now for 2.11.

I did not do the benchmarks, you might want to do them but I think there should be a performance increase. At least for some of the tests.

Pierre-Sassoulas
Pierre-Sassoulas approved these changes Aug 30, 2021
View reviewed changes
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thanks a lot, great checker and great clean up too !

@Pierre-Sassoulas
Copy link
Member

Regarding the performance increase, I also think that it should increase because of f-string but we'd switch to f-string even with a sligth decrease in performance because of the readability and modernity :)

@Pierre-Sassoulas Pierre-Sassoulas merged commit 66ffcbc into pylint-dev:main Aug 30, 2021
@DanielNoord DanielNoord deleted the f-string-convention branch August 30, 2021 07:38
@cdce8p
Copy link
Member

cdce8p commented Aug 30, 2021

I don't think this should have been part of the StringChecker. It's only a recommendation, so at the very least this should go into the RecommendationChecker.

IMO this should even move to a new extension. str.format is still perfectly valid and in some cases even the more readable option!

@cdce8p cdce8p mentioned this pull request Aug 30, 2021
Closed
4 tasks
@MuellerSeb MuellerSeb mentioned this pull request Sep 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pierre-Sassoulas Pierre-Sassoulas Pierre-Sassoulas approved these changes

Assignees
No one assigned
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Consider nudging users towards using f-strings
4 participants
@DanielNoord @coveralls @Pierre-Sassoulas @cdce8p