-
Notifications
You must be signed in to change notification settings - Fork 10
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 pragma error in RemoveUnusedImports #102
Conversation
Codecov Report
@@ Coverage Diff @@
## main #102 +/- ##
==========================================
+ Coverage 95.51% 95.63% +0.11%
==========================================
Files 60 60
Lines 2432 2451 +19
==========================================
+ Hits 2323 2344 +21
+ Misses 109 107 -2
|
@@ -45,6 +45,9 @@ def __init__(self, codemod_context: CodemodContext, *codemod_args): | |||
BaseCodemod.__init__(self, *codemod_args) | |||
|
|||
def transform_module_impl(self, tree: cst.Module) -> cst.Module: | |||
# Do nothing in __init__.py files | |||
if self.file_context.file_path.name == "__init__.py": |
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.
Is this getting tested by a unit test already?
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.
Good catch, I'll add one.
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.
This looks good but I would expect to see tests that cover the except
cases.
"unused-import" in p.messages or "W0611" in p.messages | ||
): | ||
return True | ||
# If pragma parse fails, ignore |
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.
Do we have a test for this case? It would be useful to add the case that caused the error in the first place.
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.
It was failing because of a # NOQA
comment in one of the files in pytest
. The thing is, any comment that is not recognized as a pragma by pylint
will raise an exception (note that we use a function from pylint
directly). We have no control of those, hence the catch-all try
-except
.
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.
We should probably honor # noqa
as well for this codemod but that doesn't have to happen in this PR.
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.
We do (and have tests for it). I've also fixed an issue in this PR where the regular expression that detects the # noqa
pragma was case sensitive.
"unused-import" in p.messages or "W0611" in p.messages | ||
): | ||
return True | ||
# If pragma parse fails, ignore |
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.
Do we have a test for this case?
Overview
Fixes errors with pragma parsing in RemoveUnusedImports. Also added a check to ignore
__init__.py
files.