Skip to content

[logging-too-few-args] Raise for format strings with no interpolation at all #10321

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

Merged
merged 2 commits into from
Mar 30, 2025

Conversation

Pierre-Sassoulas
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas commented Mar 30, 2025

Type of Changes

Type
βœ“ πŸ› Bug fix
βœ“ πŸ”¨ Refactoring

Description

The original decision was taken in db8b3a4 during implementation with a different rational ("If no args were supplied, then all format strings are valid don't check any further.") and was not discussed again when the comment was updated in #2713. I think the new behavior make sense especially considering that the primer shows 4 false negative in home-assistant.

Taken from the initial implementation in #10108 based on indication from #9999

Closes #9999

…ated too

The original decision was taken in db8b3a4 during implementation with
a different rational ("If no args were supplied, then all format strings
are valid don't check any further.") and was not discussed again when the
comment was updated in #2713. I think the new behavior make sense especially
considering that the primer shows 4 false negative in home-assistant.

Co-authored-by: Alex Prabhat Bara <[email protected]>
@Pierre-Sassoulas Pierre-Sassoulas added the False Negative πŸ¦‹ No message is emitted but something is wrong with the code label Mar 30, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 4.0.0 milestone Mar 30, 2025
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in the logging checker by ensuring that format strings with insufficient interpolation arguments are flagged, and it refactors the logic to handle such cases consistently.

  • Added new test files for both old and new-style logging usage to demonstrate and verify the improved behavior.
  • Removed an early exit in the logging checker to enforce proper argument checking even when no arguments are supplied.

Reviewed Changes

Copilot reviewed 4 out of 9 changed files in this pull request and generated no comments.

File Description
tests/functional/l/logging/logging_too_few_args_old_style.py Added tests for old style logging calls with too few arguments
tests/functional/l/logging/logging_too_few_args_new_style.py Added tests for new style logging calls with too few arguments
pylint/checkers/logging.py Removed the early return to update the logic for format string checks
Files not reviewed (5)
  • doc/whatsnew/fragments/9999.false_negative: Language not supported
  • tests/functional/l/logging/logging_too_few_args.txt: Language not supported
  • tests/functional/l/logging/logging_too_few_args_new_style.txt: Language not supported
  • tests/functional/l/logging/logging_too_few_args_old_style.rc: Language not supported
  • tests/functional/l/logging/logging_too_few_args_old_style.txt: Language not supported
Comments suppressed due to low confidence (3)

tests/functional/l/logging/logging_too_few_args_old_style.py:5

  • [nitpick] Consider adding assertions or expected warning message verification to ensure that this logging call produces the expected warning, instead of relying solely on the function call.
logging.error("%s, %s", 1)  # [logging-too-few-args]

tests/functional/l/logging/logging_too_few_args_new_style.py:7

  • [nitpick] Consider adding explicit assertions to verify that this logging call triggers the expected warning, which would improve the robustness of the test coverage.
logging.error("{}")  # [logging-too-few-args]

pylint/checkers/logging.py:328

  • Ensure that the removal of this early return does not lead to unexpected behavior when no logging arguments are supplied; verify that subsequent logic handles these cases appropriately.
if not num_args:

This comment has been minimized.

@Pierre-Sassoulas Pierre-Sassoulas requested a review from zenlyj March 30, 2025 12:55
Copy link

codecov bot commented Mar 30, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.89%. Comparing base (3b9e2d2) to head (f7200cf).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #10321      +/-   ##
==========================================
- Coverage   95.89%   95.89%   -0.01%     
==========================================
  Files         175      175              
  Lines       19082    19080       -2     
==========================================
- Hits        18299    18297       -2     
  Misses        783      783              
Files with missing lines Coverage Ξ”
pylint/checkers/logging.py 94.66% <ΓΈ> (-0.08%) ⬇️
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on home-assistant:
The following messages are now emitted:

  1. logging-too-few-args:
    Not enough arguments for logging format string
    https://github.com/home-assistant/core/blob/09f6246d1b7693393332535866e02fb203fb689d/homeassistant/components/homeworks/config_flow.py#L175
  2. logging-too-few-args:
    Not enough arguments for logging format string
    https://github.com/home-assistant/core/blob/09f6246d1b7693393332535866e02fb203fb689d/homeassistant/components/shopping_list/__init__.py#L242
  3. logging-too-few-args:
    Not enough arguments for logging format string
    https://github.com/home-assistant/core/blob/09f6246d1b7693393332535866e02fb203fb689d/homeassistant/components/snoo/config_flow.py#L53
  4. logging-too-few-args:
    Not enough arguments for logging format string
    https://github.com/home-assistant/core/blob/09f6246d1b7693393332535866e02fb203fb689d/homeassistant/components/sisyphus/light.py#L88
  5. logging-too-few-args:
    Not enough arguments for logging format string
    https://github.com/home-assistant/core/blob/09f6246d1b7693393332535866e02fb203fb689d/homeassistant/components/sisyphus/light.py#L94

This comment was generated for commit f7200cf

@DanielNoord DanielNoord merged commit c236745 into main Mar 30, 2025
45 checks passed
@DanielNoord DanielNoord deleted the fix-too-few-args branch March 30, 2025 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Negative πŸ¦‹ No message is emitted but something is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect missing arguments in logging format strings
2 participants