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

Partial typing of imports.py #6982

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

@coveralls
Copy link

coveralls commented Jun 20, 2022

Pull Request Test Coverage Report for Build 2663433329

  • 38 of 38 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 95.372%

Totals Coverage Status
Change from base Build 2663295382: 0.001%
Covered Lines: 16797
Relevant Lines: 17612

πŸ’› - Coveralls

Copy link
Member

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

Maybe we could create an alias for nodes.Import | nodes.ImportFrom ?

Comment on lines +554 to +563
node: nodes.If
| nodes.Expr
| nodes.Comprehension
| nodes.IfExp
| nodes.Assign
| nodes.AssignAttr
| nodes.TryExcept
| nodes.TryFinally,
Copy link
Member

Choose a reason for hiding this comment

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

If this can be everything except an import node it could also be a classDef or a lot of things, right ? Maybe NodeNG would make sense ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See L577. I thought I would be explicit just like we normally are for visit_ methods.

That said L622 shows that L602 is incorrect.

I'm drafting as I'd like to fix that in this PR.

@DanielNoord DanielNoord marked this pull request as draft June 20, 2022 07:33
@DanielNoord
Copy link
Collaborator Author

Maybe we could create an alias for nodes.Import | nodes.ImportFrom ?

There is mixins.ImportFrom in astroid but since that doesn't inherit from NodeNG it doesn't really work.

Let's put pylint-dev/astroid#1633 in 2.12 and do the necessary refactoring as well. Marc just gave his thumbs up for the issue so I think he is also okay with my proposal to start inheriting from NodeNG for the mixins.

TLDR: Drafting as we need astroid 2.12 with additional changes + a fix for visit_functiondef in this module.

@DanielNoord DanielNoord added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jun 20, 2022
@github-actions

This comment has been minimized.

@DanielNoord DanielNoord removed the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Jul 13, 2022
@DanielNoord DanielNoord marked this pull request as ready for review July 13, 2022 12:37
Copy link
Member

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

If mypy is happy, I'm happy too πŸ˜„

@DanielNoord DanielNoord merged commit 65543fd into pylint-dev:main Jul 13, 2022
@DanielNoord DanielNoord deleted the typing-checkers-2 branch July 13, 2022 12:50
@github-actions
Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on django:
The following messages are now emitted:

  1. invalid-name:
    Attribute name "Meta" doesn't conform to '[a-z_][a-z0-9_]{2,}$' pattern
    https://github.com/django/django/blob/main/django/db/models/base.py#L360
  2. no-value-for-parameter:
    No value for argument 'cls' in classmethod call
    https://github.com/django/django/blob/main/django/utils/deconstruct.py#L17

The following messages are no longer emitted:

  1. no-member:
    Instance of 'BaseManager' has no '_constructor_args' member
    https://github.com/django/django/blob/main/django/db/models/manager.py#L75
  2. no-member:
    Instance of 'BaseManager' has no '_constructor_args' member
    https://github.com/django/django/blob/main/django/db/models/manager.py#L76
  3. no-member:
    Instance of 'BaseManager' has no '_constructor_args' member
    https://github.com/django/django/blob/main/django/db/models/manager.py#L169

Effect on pandas:
The following messages are now emitted:

  1. redefined-variable-type:
    Redefinition of columns type from pandas.core.indexes.range.RangeIndex to pandas.core.indexes.base.Index
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/internals/construction.py#L860
  2. redefined-variable-type:
    Redefinition of new_target type from pandas.core.indexes.category.CategoricalIndex to pandas.core.indexes.base.Index
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/category.py#L455

The following messages are no longer emitted:

  1. redefined-variable-type:
    Redefinition of index type from pandas.core.indexes.base.Index to pandas.core.indexes.range.RangeIndex
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/series.py#L456
  2. redefined-variable-type:
    Redefinition of columns type from pandas.core.indexes.base.Index to pandas.core.indexes.range.RangeIndex
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/internals/construction.py#L850
  3. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/floating.py#L141
  4. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/floating.py#L148
  5. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L160
  6. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L167
  7. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L174
  8. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L181
  9. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L188
  10. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L195
  11. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L202
  12. empty-docstring:
    Empty class docstring
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/arrays/integer.py#L209
  13. redefined-variable-type:
    Redefinition of new_target type from pandas.core.indexes.base.Index to pandas.core.indexes.category.CategoricalIndex
    https://github.com/pandas-dev/pandas/blob/main/pandas/core/indexes/category.py#L442

Effect on pytest:
The following messages are now emitted:

  1. consider-using-dict-items:
    Consider iterating with .items()
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/reports.py#L513
  2. redefined-variable-type:
    Redefinition of ret type from str to _pytest.config.ExitCode
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/pytester.py#L1145
  3. redefined-variable-type:
    Redefinition of ret type from int to _pytest.config.ExitCode
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/pytester.py#L1429
  4. no-name-in-module:
    No name 'PytestReturnNotNoneWarning' in module '_pytest.warning_types'
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/python.py#L80
  5. redefined-variable-type:
    Redefinition of importhook type from _distutils_hack.DistutilsMetaFinder to _pytest.assertion.DummyRewriteHook
    https://github.com/pytest-dev/pytest/blob/main/src/_pytest/assertion/__init__.py#L69

Effect on sentry:
The following messages are now emitted:

  1. redefined-variable-type:
    Redefinition of culprit type from str to list
    https://github.com/getsentry/sentry/blob/master/src/sentry/culprit.py#L38
  2. consider-iterating-dictionary:
    Consider iterating the dictionary directly instead of calling .keys()
    https://github.com/getsentry/sentry/blob/master/src/sentry/web/frontend/error_page_embed.py#L138
  3. no-member:
    Instance of 'module' has no 'file' member
    https://github.com/getsentry/sentry/blob/master/src/sentry/utils/distutils/commands/base.py#L37

The following messages are no longer emitted:

  1. unused-import:
    Unused ActorTuple imported from sentry.models
    https://github.com/getsentry/sentry/blob/master/src/sentry/models/projectownership.py#L9
  2. not-callable:
    fmt is not callable
    https://github.com/getsentry/sentry/blob/master/src/sentry/utils/snuba.py#L1316
  3. no-member:
    Instance of 'str' has no 'objects' member
    https://github.com/getsentry/sentry/blob/master/src/sentry/utils/snuba.py#L1316
  4. redefined-variable-type:
    Redefinition of initials type from str to django.utils.safestring.SafeString
    https://github.com/getsentry/sentry/blob/master/src/sentry/utils/avatar.py#L77

This comment was generated for commit 58e2ab3

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

Successfully merging this pull request may close these issues.

3 participants