Skip to content

Ignore the blended messages#722

Merged
sbrunner merged 1 commit intomasterfrom
blending-ignore
Feb 5, 2025
Merged

Ignore the blended messages#722
sbrunner merged 1 commit intomasterfrom
blending-ignore

Conversation

@sbrunner
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but how we use the regexp can also be linked to the tool :-)

@sbrunner sbrunner changed the title Ignore the blended messagges Ignore the blended messages Jan 31, 2025
@sbrunner sbrunner merged commit 5e06adc into master Feb 5, 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