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

Ignore types for stuff imported from pip #171

Merged
merged 1 commit into from
May 17, 2021

Conversation

frenzymadness
Copy link
Collaborator

There are differences in the versions of pip which mypy cannot handle.

The problem we have here is caused by these facts:

  1. New pip got a lot of changes related to typing - see for example pypa/pip@a6392bd and pypa/pip@0945809
  2. Mypy checks its types against the main pip, not the one we install via the pytest fixture. It might make sense to have a specific tox environment for that so we don't run the exact same tests in tens of environments. I'm gonna prepare a PR for that as well.
  3. Fedora doesn't have the latest pip yet but Windows seems to use the latest one. This is the reason why it appeared on Windows first.

Unfortunately, it seems that there is no easy solution for that now. There are many issues in mypy complaining about handling the conditional imports or as we have here the try/except approach for imports, see the main issue: python/mypy#1297 and many more.

A much harder approach to work around this problem might be to use if/else instead of try/except for imports but it's not always possible to import the right thing from the right place based just on the version.

Let me know what do you think about it.

P.S.: I've completely installed a windows machine just to realize the only thing I need is to upgrade pip in a virtual environment.

@sesheta sesheta requested a review from KPostOffice May 14, 2021 08:55
@sesheta sesheta added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 14, 2021
@sesheta
Copy link
Member

sesheta commented May 14, 2021

Pre-Commit Test failed! Click here
[INFO] Initializing environment for git://github.com/Lucas-C/pre-commit-hooks.
[INFO] Initializing environment for git://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for git://github.com/pycqa/pydocstyle.git.
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Initializing environment for https://github.com/pre-commit/mirrors-mypy.
[INFO] Initializing environment for https://github.com/psf/black.
[INFO] Initializing environment for https://gitlab.com/PyCQA/flake8.
[INFO] Initializing environment for https://gitlab.com/PyCQA/flake8:pep8-naming.
[INFO] Installing environment for git://github.com/Lucas-C/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for git://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for git://github.com/pycqa/pydocstyle.git.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/pre-commit/mirrors-mypy.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://github.com/psf/black.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
[INFO] Installing environment for https://gitlab.com/PyCQA/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
Tabs remover.............................................................Passed
Trim Trailing Whitespace.................................................Passed
Check for merge conflicts................................................Passed
Fix End of Files.........................................................Passed
Check for added large files..............................................Passed
check BOM - deprecated: use fix-byte-order-marker........................Passed
Check for case conflicts.................................................Passed
Check docstring is first.................................................Passed
Check JSON...............................................................Passed
Detect Private Key.......................................................Passed
Check python ast.........................................................Passed
Debug Statements (Python)................................................Passed
pydocstyle...............................................................Passed
Check Toml...............................................................Passed
Check Yaml...............................................................Passed
Fix End of Files.........................................................Passed
Trim Trailing Whitespace.................................................Passed
mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

micropipenv.py:61: error: unused 'type: ignore' comment
micropipenv.py:67: error: unused 'type: ignore' comment
micropipenv.py:69: error: unused 'type: ignore' comment
micropipenv.py:74: error: unused 'type: ignore' comment
micropipenv.py:76: error: unused 'type: ignore' comment
micropipenv.py:370: error: unused 'type: ignore' comment
micropipenv.py:384: error: unused 'type: ignore' comment
micropipenv.py:388: error: unused 'type: ignore' comment
micropipenv.py:414: error: unused 'type: ignore' comment
micropipenv.py:425: error: unused 'type: ignore' comment
micropipenv.py:437: error: unused 'type: ignore' comment
micropipenv.py:450: error: unused 'type: ignore' comment
micropipenv.py:451: error: unused 'type: ignore' comment
micropipenv.py:463: error: unused 'type: ignore' comment
micropipenv.py:465: error: unused 'type: ignore' comment
micropipenv.py:478: error: unused 'type: ignore' comment
micropipenv.py:488: error: unused 'type: ignore' comment
micropipenv.py:552: error: unused 'type: ignore' comment
Found 18 errors in 1 file (checked 2 source files)

black....................................................................Passed
flake8...................................................................Passed

There are differences in the versions of pip which mypy cannot handle.
Copy link
Collaborator

@fridex fridex left a comment

Choose a reason for hiding this comment

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

Let me know what do you think about it.

First, thanks for this work 👍🏻

Generally, I do not like ignoring typing if it is used, but it sounds to me we have no easy option here. Maintaining proper typing inside micropipenv code sounds like a challenging task based on your observations. I'm fine with merging this and keeping enforced typing for functions we export outside of micropipenv (end elsewhere where possible). Given the fact the testsuite is pretty extensive and covers logic required for different pip versions exposing different API, I think we can have this in.

Thanks again for this work.

P.S.: I've completely installed a windows machine just to realize the only thing I need is to upgrade pip in a virtual environment.

PS: I hope you switch back to fedora :)

@sesheta
Copy link
Member

sesheta commented May 17, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fridex

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2021
@fridex fridex merged commit 34ef657 into thoth-station:master May 17, 2021
@frenzymadness frenzymadness deleted the fix_types branch May 19, 2021 04:35
@frenzymadness
Copy link
Collaborator Author

I'm gonna follow the mentioned mypy issues and once fixed, we can stop ignoring some of the types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants