Skip to content

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 97 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