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

Ignore the blended messages #722

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ignore the blended messages #722

wants to merge 2 commits into from

Conversation

sbrunner
Copy link
Member

Description

When one of the blended messages is ignored, ignore the other.

Related Issue

Fix #720

Motivation and Context

  • Avoid confusion
  • Don't have multiple ignore on the same line

How Has This Been Tested?

New tests added

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@sbrunner sbrunner force-pushed the blending-ignore branch 4 times, most recently from e232918 to 4b1f364 Compare January 30, 2025 15:19
@sbrunner sbrunner marked this pull request as ready for review January 30, 2025 15:35
Copy link
Collaborator

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

Looks great !

Comment on lines -84 to -90
_PYLINT_EQUIVALENTS = {
# TODO: blending has this info already?
"unused-import": (
("pyflakes", "FL0001"),
("frosted", "E101"),
)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice cleanup.

Comment on lines 15 to 16
_IGNORE_RE = re.compile(r"#\s*noqa:([^#]*[^# ])(?:\s*#.*)?$", re.IGNORECASE)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might want to add this to the ToolBase class for clarity ?

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The change is nice but I meant add the regex as an attribute in the ToolBase class to specify the regex for a tool noqa/disable explicitly (make it part of the ToolBase's API instead of creating a constant for each tool).

Copy link
Member Author

Choose a reason for hiding this comment

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

Like the new commit?
I'm nor relay convinced, but it does the job :-)
(I should update the tests!)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeap ! My reasoning is that the regex is strongly linked to a particular tool, but I don't mind if you revert, it's a nit.

@sbrunner sbrunner changed the title Ignore the blended messagges Ignore the blended messages Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Blending and ignore
2 participants