Skip to content

Conversation

progval
Copy link
Contributor

@progval progval commented May 27, 2023

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/4.0.x labels May 27, 2023
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.17.5 milestone May 27, 2023
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?

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.

@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.

@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.

@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
jacobtylerwalls added a commit that referenced this pull request Jul 8, 2023
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Backported False Positive 🦟 A message is emitted but nothing is wrong with the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants