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

Add format-string-without-interpolation checker #4794

Merged
merged 7 commits into from
Aug 4, 2021

Conversation

DanielNoord
Copy link
Collaborator

  • 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

This adds a checker that checks strings with '%' or format() applied to them.
If no variables to be replaced are found the warning is emitted.
Closes #4042

This adds a checker that checks strings with '%' or format() applied to them.
If no variables to be replaced are found the warning is emitted.
Closes pylint-dev#4042
@DanielNoord
Copy link
Collaborator Author

I'm not sure how to handle the failing check in pylint/config/__init__.py on line 129.
Seems to me that the checker has identified a problem in the current code, as globals() is not interpolated into the string. Do we want to do this? Or should we add the checker to the ignore list of this file?

@coveralls
Copy link

coveralls commented Aug 3, 2021

Coverage Status

Coverage increased (+0.003%) to 92.59% when pulling 0d0b145 on DanielNoord:format-string into a71cfe1 on PyCQA:main.

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.

Look good ! I only have a small comment.

pylint/checkers/strings.py Show resolved Hide resolved
@Pierre-Sassoulas Pierre-Sassoulas added the Enhancement ✨ Improvement to a component label Aug 3, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.10.0 milestone Aug 3, 2021
@Pierre-Sassoulas
Copy link
Member

Regarding config init : Very good news we discovered a problem in pylint with this new checker ? 😄 I think we can remove globals() from this string (to keep the same behavior at least). I think this would help during debug but clearly no one used this in a long time or we'd have a bug report at this point.

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
@DanielNoord
Copy link
Collaborator Author

This is probably me not understanding how old_names work, but now all tests are failing as all too-many-format-args seem to have become format-string-without-interpolation. Probably my mistake, but not sure what happened here..

@Pierre-Sassoulas
Copy link
Member

Ho no sorry my bad. It's not possible to rename only a part of it. We'd have to rename both. It means we'd need to rename [too-many-format-args too. I don't know about that. this seems hacky. Maybe if we find a better name we can kill two bird with one stone ?

@DanielNoord
Copy link
Collaborator Author

Hmm too-many-format-args seems pretty good as is. Is there no way to keep it the same?
Perhaps additional-format-args? Although I think too-many-format-args is still better, especially considering too-few-format-args is also a thing..

@Pierre-Sassoulas
Copy link
Member

Another way would be to refactor the MessageIdStore so it's possible to partially rename a message. I think the consequences is that you'd need to instantly rename all the messages that need to be renamed when partially renaming a message or you'd get the old name (kinda defeating the purpose of it in the first place). Beside if you have a too-many-format-args this is most likely an error so you won't disable it very often. I think reverting the old_names I asked is the sane decision here. Sorry for wasting your time with this comment.

@DanielNoord
Copy link
Collaborator Author

No problem, I can imagine why you wanted to add old_message initially!

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.

Thank you for reverting ! 👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit ea7f39e into pylint-dev:main Aug 4, 2021
@DanielNoord DanielNoord deleted the format-string branch August 4, 2021 20:45
@DanielNoord
Copy link
Collaborator Author

Not that I care too much about it, but it seems like the Copyright lines at the top of a document are not updated automatically. Should I create a PR to do so?

@Pierre-Sassoulas
Copy link
Member

It's done automatically but during release (check doc/release.md). It's a pretty heavy calculation using copyrite, also it changes the order of line randomly which is not ideal but not a real problem either. This lib could be optimized or changed but it never bothered me enough to actually do anything. I you have a better solution on the shelf I'm open to suggestion :)

@DanielNoord
Copy link
Collaborator Author

Ah okay! Don't really have a better solution, but will keep my eyes open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

format-string inconsistencies and bugs
3 participants