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

Use type alias for temporal intervals #744

Merged

Conversation

duckontheweb
Copy link
Contributor

@duckontheweb duckontheweb commented Feb 3, 2022

Related Issue(s):

Description:

Adds 2 type variables that are used in the TemporalExtent class:

  • TemporalIntervals - This is used for properly formatted STAC temporal intervals (i.e. a list of lists)
  • TemporalIntervalsLike - This is used for the intervals argument to TemporalExtent where we allow either a list of lists or a list of optional datetimes and convert it into a list of lists.

I tried using a covariant TypeVar as I had initially suggested in #586 (comment), but that did not fix the type errors. Using a Union type of all the possible combinations seemed to be the only way to make mypy happy.

Also note that this adds a test whose only purpose is type checking when we run mypy. If there are suggestions on a better place to put this kind of check I'm happy to change that.

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • Documentation has been updated to reflect changes, if applicable
  • 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.

@duckontheweb
Copy link
Contributor Author

cc: @cuttlefish

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2022

Codecov Report

Merging #744 (0a026b4) into main (4b30217) will increase coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #744   +/-   ##
=======================================
  Coverage   94.33%   94.34%           
=======================================
  Files          77       77           
  Lines       11285    11291    +6     
  Branches     1347     1347           
=======================================
+ Hits        10646    10652    +6     
  Misses        459      459           
  Partials      180      180           
Impacted Files Coverage Δ
pystac/collection.py 94.26% <80.00%> (+0.04%) ⬆️
tests/test_collection.py 100.00% <100.00%> (ø)

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 4b30217...0a026b4. Read the comment docs.

Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

I did have one question about the type union but I'm probably not understanding it correctly so approving so I don't block if I'm wrong :-).

pystac/collection.py Outdated Show resolved Hide resolved
duckontheweb added a commit to duckontheweb/pystac that referenced this pull request Feb 3, 2022
@duckontheweb duckontheweb added this to the 1.4.0 milestone Feb 8, 2022
@duckontheweb duckontheweb force-pushed the fix/586-temporal-interval-typing branch from 0a026b4 to 696cfc4 Compare February 15, 2022 20:35
@duckontheweb duckontheweb merged commit 8b8ad7e into stac-utils:main Feb 16, 2022
@duckontheweb duckontheweb deleted the fix/586-temporal-interval-typing branch February 16, 2022 18:00
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.

Type issues with TemporalExtent
3 participants