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

Avoid consider-using-f-string on modulos with brackets in template #8720

Merged
merged 6 commits into from
Jul 8, 2023
Merged

Avoid consider-using-f-string on modulos with brackets in template #8720

merged 6 commits into from
Jul 8, 2023

Conversation

progval
Copy link
Contributor

@progval progval commented May 27, 2023 •
edited
Loading

Type of Changes

Type
✓ 🐛 Bug fix

Description

Brackets can be inconvenient in f-string expressions.

For example, "{%s}%s" % (namespace, tag) (idiomatic when using ElementTree) would be f"{{{namespace}}}{tag}" as a f-string

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code backport maintenance/3.3.x labels May 27, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.5 milestone May 27, 2023
Pierre-Sassoulas
Pierre-Sassoulas reviewed May 27, 2023
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.

I think it make sense, I'll wait for another opinion before merging.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label May 27, 2023
@progval
Copy link
Contributor Author

progval commented May 27, 2023

hmm, what should I do about this failing test?

pylint/tests/functional/r/regression_02/regression_8109.py

Line 14 in c5f3ff4

number_format = "{:,.%sf}" % self.round # [consider-using-f-string]

@Pierre-Sassoulas
Copy link
Member

Hmm it seems #8109 is a regression tests about a crash and not about actually raising consider-using-f-string in this case. So I would be okay not raising it anymore. Something to consider during review though.

@github-actions

This comment has been minimized.

Sign in to view
@Pierre-Sassoulas Pierre-Sassoulas added the Waiting on author Indicate that maintainers are waiting for a message of the author label Jul 8, 2023
@Pierre-Sassoulas
Copy link
Member

@progval do you think you'll be able to upgrade the tests to make the CI succeed ? :)

@progval
Copy link
Contributor Author

progval commented Jul 8, 2023

Sorry, thanks for the reminder. Done now

@Pierre-Sassoulas Pierre-Sassoulas removed the Waiting on author Indicate that maintainers are waiting for a message of the author label Jul 8, 2023
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

🤖 Effect of this PR on checked open source code: 🤖

Effect on astroid:
The following messages are no longer emitted:

  1. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/pylint-dev/astroid/blob/e91a3b5c9ceaf3171f0915cff95d40b9f05cfdee/astroid/nodes/as_string.py#L228
  2. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/pylint-dev/astroid/blob/e91a3b5c9ceaf3171f0915cff95d40b9f05cfdee/astroid/nodes/as_string.py#L324
  3. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/pylint-dev/astroid/blob/e91a3b5c9ceaf3171f0915cff95d40b9f05cfdee/astroid/nodes/as_string.py#L465

Effect on home-assistant:
The following messages are no longer emitted:

  1. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/home-assistant/core/blob/c2ccd185289640caf806f7fe6701b842d3a50068/homeassistant/components/verisure/lock.py#L115
  2. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/home-assistant/core/blob/c2ccd185289640caf806f7fe6701b842d3a50068/homeassistant/components/emoncms_history/__init__.py#L88

Effect on django:
The following messages are no longer emitted:

  1. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/contrib/gis/management/commands/ogrinspect.py#L150
  2. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/contrib/gis/serializers/geojson.py#L29
  3. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/db/migrations/writer.py#L30
  4. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/db/migrations/serializer.py#L121
  5. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/utils/http.py#L34
  6. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/utils/http.py#L35
  7. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/utils/http.py#L36
  8. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/django/django/blob/2584783f46922bcb456ceb9700a3726314df65d3/django/template/loader_tags.py#L73

Effect on pandas:
The following messages are now emitted:

  1. useless-suppression:
    Useless suppression of 'consider-using-f-string'
    https://github.com/pandas-dev/pandas/blob/457690995ccbfc5b8eee80a0818d62070d078bcf/pandas/tests/io/formats/test_to_latex.py#L1314

The following messages are no longer emitted:

  1. suppressed-message:
    Suppressed 'consider-using-f-string' (from line 1314)
    https://github.com/pandas-dev/pandas/blob/457690995ccbfc5b8eee80a0818d62070d078bcf/pandas/tests/io/formats/test_to_latex.py#L1314

Effect on sentry:
The following messages are no longer emitted:

  1. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/getsentry/sentry/blob/4dd5e1526d7192e94f393a8f35a7399923d5b83e/src/sentry/utils/urls.py#L58
  2. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/getsentry/sentry/blob/4dd5e1526d7192e94f393a8f35a7399923d5b83e/src/sentry/utils/numbers.py#L77
  3. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/getsentry/sentry/blob/4dd5e1526d7192e94f393a8f35a7399923d5b83e/src/sentry/utils/pytest/relay.py#L101
  4. consider-using-f-string:
    Formatting a regular string which could be an f-string
    https://github.com/getsentry/sentry/blob/4dd5e1526d7192e94f393a8f35a7399923d5b83e/src/sentry/plugins/base/configuration.py#L27

This comment was generated for commit 2139564

@jacobtylerwalls jacobtylerwalls removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jul 8, 2023
@jacobtylerwalls jacobtylerwalls merged commit ab77b98 into pylint-dev:main Jul 8, 2023
@jacobtylerwalls
Copy link
Member

Thanks for the contribution!

@github-actions
Copy link
Contributor

github-actions bot commented Jul 8, 2023

The backport to maintenance/2.17.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-maintenance/2.17.x maintenance/2.17.x
# Navigate to the new working tree
cd .worktrees/backport-maintenance/2.17.x
# Create a new branch
git switch --create backport-8720-to-maintenance/2.17.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ab77b988fe9352166faa2cb9126a5f2569403d98
# Push it to GitHub
git push --set-upstream origin backport-8720-to-maintenance/2.17.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-maintenance/2.17.x

Then, create a pull request where the base branch is maintenance/2.17.x and the compare/head branch is backport-8720-to-maintenance/2.17.x.

jacobtylerwalls pushed a commit to jacobtylerwalls/pylint that referenced this pull request Jul 8, 2023
@progval @jacobtylerwalls
Avoid consider-using-f-string on modulos with brackets in template (p…
04be23b 
…ylint-dev#8720)

Brackets can be inconvenient in f-string expressions.
@jacobtylerwalls jacobtylerwalls mentioned this pull request Jul 8, 2023
Merged
@Pierre-Sassoulas
Copy link
Member

Congratulation on becoming a pylint contributor, the primer result looks pretty good on this one, that's a lot of false positives avoided ! 💪

Pierre-Sassoulas pushed a commit to jacobtylerwalls/pylint that referenced this pull request Jul 8, 2023
@progval @Pierre-Sassoulas
Avoid consider-using-f-string on modulos with brackets in template (p…
17fbcfe 
…ylint-dev#8720)

Brackets can be inconvenient in f-string expressions.
jacobtylerwalls added a commit that referenced this pull request Jul 8, 2023
@jacobtylerwalls @progval @Pierre-Sassoulas
[Backport maintenance/2.17.x] Avoid consider-using-f-string on modu…
1928589 
…los with brackets in template … (#8836)

* Avoid `consider-using-f-string` on modulos with brackets in template (#8720)

Brackets can be inconvenient in f-string expressions.

Co-authored-by: Val Lorentz <[email protected]>
Co-authored-by: Pierre Sassoulas <[email protected]>
@Pierre-Sassoulas Pierre-Sassoulas mentioned this pull request Jul 26, 2023
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 left review comments

@jacobtylerwalls jacobtylerwalls jacobtylerwalls approved these changes

Assignees
No one assigned
Labels
Backported False Positive 🦟 A message is emitted but nothing is wrong with the code
Projects
None yet
Milestone
2.17.5
Development

Successfully merging this pull request may close these issues.
3 participants
@progval @Pierre-Sassoulas @jacobtylerwalls