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

Mypy cleanup #1562

Merged
merged 27 commits into from
Mar 14, 2024
Merged

Mypy cleanup #1562

merged 27 commits into from
Mar 14, 2024

Conversation

SpacemanPaul
Copy link
Contributor

@SpacemanPaul SpacemanPaul commented Mar 12, 2024

Reason for this pull request

Get develop-1.9 branch pass mypy checks and add a github action to preserve compliance going forwards

Proposed changes

Add missing typehints; fix broken typehints; refactor some code to make more amenable to static analysis; add casts, asserts and other hints; and where all else fails add an explicit ignore directive.

Some obscure old bugs have been shaken out and fixed,

I'm not overly concerned about the codecov report - codecov doesn't cope well with this sort of extensive but superficial PR.

  • Tests added / passed
  • Fully documented, including docs/about/whats_new.rst for all changes

Copy link

codecov bot commented Mar 12, 2024

Codecov Report

Attention: Patch coverage is 93.28165% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 85.57%. Comparing base (5b0d0c7) to head (f30fa9a).

Files Patch % Lines
datacube/model/properties.py 64.51% 11 Missing ⚠️
datacube/drivers/postgis/_spatial.py 79.41% 7 Missing ⚠️
datacube/index/abstract.py 90.00% 3 Missing ⚠️
datacube/virtual/impl.py 50.00% 2 Missing ⚠️
datacube/drivers/postgis/_api.py 93.75% 1 Missing ⚠️
datacube/drivers/postgres/_api.py 94.11% 1 Missing ⚠️
datacube/utils/geometry/_base.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           develop-1.9    #1562      +/-   ##
===============================================
- Coverage        85.62%   85.57%   -0.06%     
===============================================
  Files              140      140              
  Lines            15299    15360      +61     
===============================================
+ Hits             13100    13144      +44     
- Misses            2199     2216      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SpacemanPaul SpacemanPaul marked this pull request as ready for review March 13, 2024 23:21
@SpacemanPaul SpacemanPaul requested a review from omad March 13, 2024 23:22
Copy link
Contributor

@Ariana-B Ariana-B left a comment

Choose a reason for hiding this comment

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

Some inconsistencies in usage of Union or Optional vs | syntax, as well as Mapping vs dict even considering recommended input/return annotation conventions

datacube/drivers/postgis/_spatial.py Show resolved Hide resolved
if "time" in [f.name for f in match_fields]:
return self.get_duplicates_with_time(match_fields, expressions)

group_expressions = tuple(f.alchemy_expression for f in match_fields)
group_expressions = tuple(type_cast(PgField, f).alchemy_expression for f in match_fields)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of casting here rather than providing a more specific type hint?

@@ -845,7 +838,7 @@ def clone(self, orig: Dataset, for_save=False, lookup_locations=True) -> Dataset
else:
uris = []
if len(uris) == 1:
kwargs = {"uri": uris[0]}
kwargs: dict[str, Any] = {"uri": uris[0]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we know that uris[0] will be a str?

Copy link
Contributor Author

@SpacemanPaul SpacemanPaul Mar 14, 2024

Choose a reason for hiding this comment

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

We do, but eg. kwargs["uris"] is a list[str] so the variable typehint has to be broader. Could probably be tighter than Any though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This whole file should probably be deleted

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, I was wondering about that - definitely after we bring in eo3 as a dependency. I'll have a look at internal usages and see if we can delete it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file should also probably be deleted

@SpacemanPaul
Copy link
Contributor Author

Some inconsistencies in usage of Union or Optional vs | syntax, as well as Mapping vs dict even considering recommended input/return annotation conventions

Yeah I don't pretend to have addressed all inconsistencies.

@SpacemanPaul SpacemanPaul merged commit a2f71d0 into develop-1.9 Mar 14, 2024
26 checks passed
@SpacemanPaul SpacemanPaul deleted the mypy-cleanup branch March 14, 2024 22:54
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.

None yet

2 participants