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

Deepcopy Collection properties on clone #794

Merged
merged 6 commits into from
Apr 29, 2022

Conversation

sunu
Copy link
Contributor

@sunu sunu commented Apr 23, 2022

Related Issue(s): Fixes #787

Description: Makes sure that all the mutable properties of a Collection get deepcopied when the collection is cloned. Implements clone on Summaries to make sure cloned collection will point to a new instance of Summaries after being cloned.

I have some weird mypy errors from the tests that I added. Still trying to figure out how to fix. I would love some help on that.

PR Checklist:

  • Code is formatted (run pre-commit run --all-files)
  • Tests pass (run scripts/test)
  • [NA] 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.

@sunu sunu changed the title Fix/787 deepcopy on clone Deepcopy Collection properties on clone Apr 23, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #794 (5bd15cf) into main (a6c8927) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   94.57%   94.59%   +0.01%     
==========================================
  Files          79       79              
  Lines       11660    11690      +30     
  Branches     1372     1372              
==========================================
+ Hits        11028    11058      +30     
  Misses        453      453              
  Partials      179      179              
Impacted Files Coverage Δ
pystac/catalog.py 92.15% <ø> (ø)
pystac/collection.py 94.26% <ø> (ø)
pystac/summaries.py 78.88% <100.00%> (+0.98%) ⬆️
tests/extensions/test_scientific.py 100.00% <100.00%> (ø)
tests/test_collection.py 100.00% <100.00%> (ø)
tests/test_summaries.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 a6c8927...5bd15cf. Read the comment docs.

@sunu
Copy link
Contributor Author

sunu commented Apr 24, 2022

Mypy errors should be fixed now. I've also added a changelog entry.

@sunu sunu force-pushed the fix/787-deepcopy-on-clone branch from 9345fb4 to 6520bc6 Compare April 26, 2022 04:03
@sunu
Copy link
Contributor Author

sunu commented Apr 26, 2022

Rebased the branch on main to make sure the ci uses Python 3.11.0-alpha.7. Hopefully that'll fix the failing tests on 3.11

@sunu
Copy link
Contributor Author

sunu commented Apr 29, 2022

Hi @duckontheweb, @gadomski! now that all the tests are passing, I'd love to get some review 🙂

@gadomski gadomski self-requested a review April 29, 2022 13:14
Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

Thanks for this @sunu. I had one comment on whether we really need to deepcopy the stac_extensions property. Other than that it looks good to me.

deepcopy() is unnecessary on properties that are only list of strings
@duckontheweb duckontheweb merged commit 876b60a into stac-utils:main Apr 29, 2022
@duckontheweb
Copy link
Contributor

Thanks again for the contribution @sunu !

@duckontheweb duckontheweb added this to the 1.5.0 milestone May 4, 2022
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.

Collection full_copy and clone don't actually deepcopy
4 participants