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

Improve mypy strictness #394

Merged
merged 5 commits into from
Jun 3, 2021
Merged

Conversation

l0b0
Copy link
Contributor

@l0b0 l0b0 commented Jun 2, 2021

Related Issue(s): #332

Description: Applies stricter mypy configuration to the code base

PR Checklist:

  • Code is formatted (run scripts/format)
  • Tests pass (run scripts/test)
  • This PR maintains or improves overall codebase code coverage.
  • Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

@codecov-commenter
Copy link

Codecov Report

Merging #394 (a1d50f0) into main (c2b7e0e) will decrease coverage by 0.06%.
The diff coverage is 89.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
- Coverage   89.82%   89.75%   -0.07%     
==========================================
  Files          38       38              
  Lines        4953     4959       +6     
==========================================
+ Hits         4449     4451       +2     
- Misses        504      508       +4     
Impacted Files Coverage Δ
pystac/serialization/__init__.py 86.95% <ø> (ø)
pystac/stac_io.py 71.73% <54.54%> (-0.65%) ⬇️
pystac/item.py 96.67% <86.66%> (-0.56%) ⬇️
pystac/__init__.py 97.14% <100.00%> (ø)
pystac/catalog.py 93.51% <100.00%> (ø)
pystac/collection.py 93.35% <100.00%> (ø)
pystac/extensions/base.py 89.36% <100.00%> (ø)
pystac/extensions/label.py 85.89% <100.00%> (ø)
pystac/extensions/pointcloud.py 96.61% <100.00%> (ø)
pystac/serialization/common_properties.py 94.64% <100.00%> (ø)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2b7e0e...a1d50f0. Read the comment docs.

ExtensionAlreadyExistsError,
ExtensionTypeError,
RequiredPropertyMissing,
STACValidationError,
Copy link
Member

Choose a reason for hiding this comment

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

Noting here that the type ignores were to avoid showing errors for unused imports in VS Code with Pylance, with the reportUnusedClass flag set. This was set in my configuration but not a project-level configuration, where the default Pyright behavior is to ignore this.

The better solution is to ignore reportUnusedClass in the VS Code Pylance settings (which is the default) and to ensure flake8 is set as the linter for the project; that way unused imports that are not in init.py (which are ignored via the # flake8: noqa directive) will be caught in VS Code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we going to support three different type hint linters (mypy, pylance and pyright)? That sounds like a very difficult task, especially if one of them doesn't run in CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lossyrob My understanding of your comment is that those ignores were specific to your VS Code setup and that we are okay to drop them as @l0b0 has done here. It seems like this is mostly a PSA to other VS Code users on how to handle this. Is that right?

Might also be good to get your input on #381. My feeling is that we should settle on 1 type checker, but would be good to get other input.

Copy link
Member

Choose a reason for hiding this comment

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

Pylance is backed by Pyright, so they are the same thing.

I had given a thumbs up on #381. I'd like to avoid a situation where Pyright (and therefore VS Code) thinks there's an error that passes CI, as it's great to be able to detect errors during editing, I find that to be the most accelerating change in my workflow in years - reminds me of when I had static compilation feedback working with other languages. However, a single type checker makes sense, and as mypy is more strict I'm hoping we A. don't get into the situation where pyright breaks, and B. some useful type-level code is blocked by mypy. Seems like that's not happening yet; also I don't think this project needs to fit to any of our personal preferences or workflows, so I think going with mypy only is a good choice.

You are correct in that my comment here was just a note in way of explination of why the type:ignores were there in the first place, and a documentation of working out how to better set up VS Code for this project.

@l0b0 l0b0 force-pushed the improve-mypy-strictness branch from a1d50f0 to 9293326 Compare June 2, 2021 21:14
@duckontheweb duckontheweb merged commit 4658999 into stac-utils:main Jun 3, 2021
@l0b0 l0b0 deleted the improve-mypy-strictness branch June 3, 2021 19:36
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.

4 participants