Skip to content

add annotations in pylint/checkers. #2163

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

Closed
wants to merge 4 commits into from

Conversation

sushobhit27
Copy link
Contributor

add mypy in tox.ini for only pylint/checkers.

Fixes / new features

tox.ini Outdated

setenv =
COVERAGE_FILE = {toxinidir}/.coverage.{envname}

commands =
python -Wi {envsitepackagesdir}/coverage run -m mypy --ignore-missing-imports {envsitepackagesdir}/pylint/checkers/ {posargs:}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please could this be run in a separate virtualenv to the tests. Seeing as mypy is failing at the moment, I think we should stop the mypy virtualenv running by default as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AWhetter added mypy in seperate virtualenv, however it is failing as many errors are false positive IMO. I will work on it to either fix it or ignore it depending on each error.

@coveralls
Copy link

coveralls commented Jun 4, 2018

Coverage Status

Coverage decreased (-0.6%) to 89.587% when pulling 654447a on sushobhit27:fix_2079 into db63b49 on PyCQA:master.

@PCManticore
Copy link
Contributor

@sushobhit27 I think you might need to do a rebase since some changes that are now in master appear in your branch as well. Didn't have time yet to review this PR though

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

Looks good overall. As mentioned, I think this needs to be rebased to get rid of these new additions. Also left a couple of comments with some places where I'm not sure what we're type ignoring.

COMP_VAR_RGX = None
DEFAULT_NAME_RGX = None
CLASS_ATTRIBUTE_RGX = None
CLASS_NAME_RGX = Pattern[str]
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this class behave like None when we define a variable with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will check for it.

Copy link
Contributor

Choose a reason for hiding this comment

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

should these instead be CLASS_NAME_RGX: Pattern[str]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@asottile my bad...yes that should be like that, or CLASS_NAME_RGX: Optional[Pattern[str]], if variable can have either None or Pattern[str] value. Missed to replace = with :

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately that's 3.6 syntax only so won't work for us just yet (we still support 3.4 and 3.5)

@@ -21,7 +21,7 @@

try:
import enchant
from enchant.tokenize import (get_tokenizer,
from enchant.tokenize import (get_tokenizer, # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have to ignore it here?

@@ -30,11 +30,11 @@
except ImportError:
enchant = None
# pylint: disable=old-style-class,no-init
class Filter:
class Filter: # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ python -m mypy --ignore-missing-imports checkers/spelling.py
checkers/spelling.py:33: error: Name 'Filter' already defined (possibly by an import)

python/mypy#2556
All these ignore were generally required to just suppress the false positive. Although now I think, in first pass instead of using ignore we should let the errors comes and filter out any actual errors and have a fix for them.
e.g mypy helped in finding just below two errors:

def is_error(node) -> bool:
     """return true if the function does nothing but raising an exception"""
     for child_node in node.get_children():
         if isinstance(child_node, astroid.Raise):
             return True
         return False

IMO return False at last statement should be de-idented.

Similarly in below example, although in most of cases below code should work but instead of returning None, bool should be returned explicitly.

def _is_property_decorator(decorator) -> bool:
     for infered in decorator.infer():
         if isinstance(infered, astroid.ClassDef):
             if infered.root().name == BUILTINS_NAME and infered.name == 'property':
                 return True
             for ancestor in infered.ancestors():
                 if ancestor.name == 'property' and ancestor.root().name == BUILTINS_NAME:
                     return True
     return None 

@@ -104,15 +105,15 @@
SPECIAL_METHODS_PARAMS = {
name: params
for params, methods in _SPECIAL_METHODS_PARAMS.items()
for name in methods
for name in methods # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto. what does this ignore, the iteration or the result of the dictionary creation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checkers/utils.py:108: error: "object" has no attribute "iter"; maybe "str" or "dir"? (not iterable)
somehow mypy not able to infer that method is a tuple. annotating _SPECIAL_METHODS_PARAMS might have fixed this, but didn't try it.

@PCManticore
Copy link
Contributor

Hey @sushobhit27 Do you still plan to work on this PR? Would love to get some annotations in.

@sushobhit27
Copy link
Contributor Author

Hey @sushobhit27 Do you still plan to work on this PR? Would love to get some annotations in.

@PCManticore AFAIK it was decided to postpone this activity and have annotations done in astroid first, that's why I stopped working on this PR, however if you want I can continue on it. Let me know any specific files, I need to pick first.

@PCManticore
Copy link
Contributor

@sushobhit27 We can definitely continue with this PR as well, as we'd see at least some minimal benefit in how to set up annotations in a way that works across Python 3.4 - 3.7. So let's bring it to a shippable state and after that we can take a look at astroid as well.

@sushobhit27
Copy link
Contributor Author

@PCManticore ok, I start working on it again.

@PCManticore
Copy link
Contributor

We can come back later at this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants