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

Fix linting: disable mypy --install-types #628

Merged
merged 1 commit into from
Mar 19, 2024

Conversation

JustAnotherArchivist
Copy link
Contributor

pip install .[all] already installs the third-party hints, and --install-types prompts for confirmation, breaking the CI.

`pip install .[all]` already installs the third-party hints, and `--install-types` prompts for confirmation, breaking the CI.
@JustAnotherArchivist
Copy link
Contributor Author

Example of a linting crash on the mypy step from https://github.com/jjjake/internetarchive/actions/runs/7892724829/job/21539842879:

Installing missing stub packages:
/opt/hostedtoolcache/Python/3.12.2/x64/bin/python -m pip install types-requests

Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.12.2/x64/bin/mypy", line 8, in <module>
    sys.exit(console_entry())
             ^^^^^^^^^^^^^^^
  File "/opt/hostedtoolcache/Python/3.12.2/x64/lib/python3.12/site-packages/mypy/__main__.py", line 15, in console_entry
    main()
  File "mypy/main.py", line 133, in main
  File "mypy/main.py", line 1568, in install_types
EOFError: EOF when reading a line
Install? [yN] 
Error: Process completed with exit code 1.

I'm not sure why it thinks types-requests is missing; the pip install .[all] step does install it, as expected...

@JustAnotherArchivist
Copy link
Contributor Author

@cclauss Would you mind verifying whether I'm understanding things correctly here?

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Nice!

@cclauss
Copy link
Contributor

cclauss commented Feb 14, 2024

Please consider fixing warnings at the bottom right of https://github.com/jjjake/internetarchive/actions/runs/7893419664

This should be done in a separate PR like:

@JustAnotherArchivist
Copy link
Contributor Author

JustAnotherArchivist commented Feb 14, 2024

Yeah, that was next on my list. Looks like the fix is to bump actions/checkout to version 4. But as you say, separate PR.

@@ -80,7 +80,6 @@ ignore-words-list = alers
[mypy]
exclude = ^\.git/|^__pycache__/|^docs/source/conf.py$|^old/|^build/|^dist/|\.tox
python_version = 3.9
install_types = True
Copy link
Contributor

@cclauss cclauss Feb 17, 2024

Choose a reason for hiding this comment

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

Suggested change
install_types = True
install_types = True
non_interactive = True

--install-types prompts for confirmation, breaking the CI.

As discussed in the mypy docs, this is exactly why the --non-interactive option was added so let's use 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.

Potentially. Although I think we'd ideally not have mypy install any type hint packages. I'm still puzzled why it even tries. That might indicate that something else is wrong.

Copy link
Contributor

@cclauss cclauss Feb 17, 2024

Choose a reason for hiding this comment

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

You can either tell mypy everything (constant human effort to align type hints) or allow it to guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but we already have the type hint packages in .[types], and they only change when our dependencies do. Here, it tries to install types-requests, which is already getting installed (via .[all]), yet somehow mypy can't find it...

Copy link
Contributor

@cclauss cclauss Feb 17, 2024

Choose a reason for hiding this comment

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

What happens if you modify line 69 to more exactly match https://pypi.org/project/types-requests ?

- types-requests>=2.25.0,<3.0.0
+ types-requests>=2.31.0.20240125,<3.0.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean. The version dependency on line 69 of setup.cfg matches the one on line 30 for requests (so that it should install matching versions), and we can see that it does install the latest PyPI version here.

Quoting for future reference since the Actions logs expire stupidly quickly:

Run pip install .[all]
<...>
Collecting requests<3.0.0,>=2.25.0 (from internetarchive==3.6.0)
  Downloading requests-2.31.0-py3-none-any.whl.metadata (4.6 kB)
<...>
Collecting types-requests<3.0.0,>=2.25.0 (from internetarchive==3.6.0)
  Downloading types_requests-2.31.0.20240125-py3-none-any.whl.metadata (1.8 kB)
<...>
Downloading requests-2.31.0-py3-none-any.whl (62 kB)
<...>
Downloading types_requests-2.31.0.20240125-py3-none-any.whl (14 kB)
<...>
Installing collected packages: <...>, types-requests, <...>, requests, <...>
Successfully installed <...> requests-2.31.0 <...> types-requests-2.31.0.20240125 <...>

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

install_types = True
non_interactive = True

@jjjake jjjake merged commit 33449b5 into jjjake:master Mar 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants