-
Notifications
You must be signed in to change notification settings - Fork 171
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
fix: Use pathlib.Path on the entire message.location.path #580
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you ! Would you mind adding a regression tests too, please ?
Done. To be fair these tests should have been written when the pathlib refactoring happened. |
for more information, see https://pre-commit.ci
|
||
def test_filter_messages_negative(self): | ||
workdir = Path(__file__).parent / "testdata/test_filter_messages_negative" | ||
with patch("setoptconf.source.commandline.sys.argv", ["prospector"]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkout the tests/utils.py
module, patch_execution
and patch_cli
might do what you need here and might as well reuse it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christokur I'm not sure you took carlio's comment into account, could you look into it please ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it into account yes
with patch("setoptconf.source.commandline.sys.argv", ...
and
with patch("sys.argv", ...
have different semantics which is why I did not use patch_cli
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok refactored things but be aware that pyupgrade also made changes to how typing is used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry for the delay !
Fix the support for
# noqa
Description
The
postfilter.filter_messages
does not recognize the paths from lint messages as matches for ignored paths and messages because it only matches part of the path and uses thestr
representation to compare against sets that usepathlib.Path
keys.Related Issue
#557
Motivation and Context
# noqa
support is broken and users lost the ability to suppress messages since1.8.0
How Has This Been Tested?
Use project tests.
Tested against local repo's where the bug is observed using
prospector <= 1.8.4
and# noqa
works as expected after the patch.Types of changes
Checklist: