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

Implement SAR extension. #33 #203

Merged
merged 4 commits into from
Oct 30, 2020
Merged

Conversation

schwehr
Copy link
Collaborator

@schwehr schwehr commented Oct 21, 2020

I'm new to type hints, so they aren't totally perfect. The classmethods are likely incorrect. It would be great to get help to make them be correct or we can drop them. Type hints tested with pytype.

It seems like there should be a subset of sar available for collections. e.g. this, which is definitely not correct.

@schwehr schwehr requested a review from lossyrob October 21, 2020 22:41
@lossyrob
Copy link
Member

There is a TODO for documentation, but a review request. Is this still in draft state?

@schwehr
Copy link
Collaborator Author

schwehr commented Oct 27, 2020

Yes, still a draft. I was hoping you'd take a look at the non-doc part.

But hold off on that and I'll give it another look over myself and add the docs.

Sorry I'm having so much trouble with the github workflow. The tooling feels very different than the reviews I do internally at Google.

@schwehr schwehr marked this pull request as draft October 27, 2020 17:16
@lossyrob
Copy link
Member

Gotcha - no worries. I just wasn't sure the state of things. If there's something particular you'd like me to take a look at, happy to, otherwise I think it'll shorten the review cycles if the review can happen all at once when the PR is out of draft phase.

@schwehr schwehr marked this pull request as ready for review October 28, 2020 17:10
@codecov-io
Copy link

Codecov Report

Merging #203 into develop will increase coverage by 0.69%.
The diff coverage is 99.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #203      +/-   ##
===========================================
+ Coverage    92.18%   92.88%   +0.69%     
===========================================
  Files           28       29       +1     
  Lines         3341     3515     +174     
===========================================
+ Hits          3080     3265     +185     
+ Misses         261      250      -11     
Impacted Files Coverage Δ
pystac/extensions/sar.py 99.33% <99.33%> (ø)
pystac/__init__.py 100.00% <100.00%> (ø)
pystac/extensions/version.py 98.26% <0.00%> (ø)
pystac/serialization/common_properties.py 100.00% <0.00%> (ø)
pystac/utils.py 97.70% <0.00%> (+0.05%) ⬆️
pystac/validation/stac_validator.py 92.30% <0.00%> (+0.09%) ⬆️
pystac/stac_object.py 95.04% <0.00%> (+0.15%) ⬆️
pystac/catalog.py 90.17% <0.00%> (+0.24%) ⬆️
pystac/collection.py 96.90% <0.00%> (+0.44%) ⬆️
... and 3 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 eaf8b42...0017df7. Read the comment docs.

Copy link
Member

@lossyrob lossyrob left a comment

Choose a reason for hiding this comment

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

Looks great, and good to merge. Few things:

  • I added a commit to put the API docs in
  • I created Handle setting None consistently in extensions. #230 to track making our usage of optional properties being set as None, and how that affects the apply method.
  • I created Refactor to use Enums consistently in PySTAC #231 to track making our usage of Enums consistent. You're using enums here, but they are not used in throughout the rest of the library. I think that's good for now, but we want to eventually be consistent on our usage. If you have opinions about that, please comment in the issue!

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