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

A collection of "low-hanging fruit" refactors #612

Open
wants to merge 15 commits into
base: typecheck-spatial
Choose a base branch
from

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Sep 18, 2024

Important

To be merged only after merging #605 and rebasing

I'm happy to split this in to multiple PRs, but there's a lot of disparate changes here and I didn't want to flood the queue before showing you the whole pile. Let me know!

Copy link

github-actions bot commented Sep 18, 2024

Binder 👈 Launch a binder notebook on this branch for commit 91bd9a8

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit b75d000

Binder 👈 Launch a binder notebook on this branch for commit 2e7ac19

Binder 👈 Launch a binder notebook on this branch for commit e6151d1

Binder 👈 Launch a binder notebook on this branch for commit 4dc2872

Binder 👈 Launch a binder notebook on this branch for commit a180892

Binder 👈 Launch a binder notebook on this branch for commit 88459e5

Binder 👈 Launch a binder notebook on this branch for commit 77112b7

@mfisher87 mfisher87 changed the title Low hanging refactors A collection of "low-hanging fruit" refactors Sep 18, 2024
if not hasattr(self, "_granules") or self._granules is None:
self._granules = Granules()

return self._granules
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks to me like self.granules is static and could be set at init-time as a class attribute, but then we wouldn't get this awesome docstring. I changed the method to a cached_property to, IMO, get the best qualities of both.

@mfisher87 mfisher87 force-pushed the low-hanging-refactors branch from 91bd9a8 to b75d000 Compare September 18, 2024 01:24
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 79.10448% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.83%. Comparing base (7675584) to head (77112b7).

Files with missing lines Patch % Lines
icepyx/core/is2ref.py 54.54% 9 Missing and 1 partial ⚠️
icepyx/core/query.py 87.09% 4 Missing ⚠️
Additional details and impacted files
@@                  Coverage Diff                  @@
##           typecheck-spatial     #612      +/-   ##
=====================================================
+ Coverage              67.76%   67.83%   +0.06%     
=====================================================
  Files                     36       37       +1     
  Lines                   3149     3146       -3     
  Branches                 430      426       -4     
=====================================================
  Hits                    2134     2134              
+ Misses                   931      928       -3     
  Partials                  84       84              

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

@trey-stafford
Copy link
Contributor

Looks like this PR is ready-to-go pending #605

pyproject.toml Outdated Show resolved Hide resolved
@JessicaS11
Copy link
Member

Looks like this PR is ready-to-go pending #605

Any idea why this shouldn't go directly into development?

@trey-stafford
Copy link
Contributor

Looks like this PR is ready-to-go pending #605

Any idea why this shouldn't go directly into development?

This work is based on what's in #605 . We might be able to pull out the changes from this PR independently if we decide not to move forward with #605 though.

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