Skip to content

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!

@github-actions
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
@codecov
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

❌ 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).
⚠️ Report is 1 commits behind head on typecheck-spatial.

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@trey-stafford
Copy link
Contributor

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

@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.

@JessicaS11
Copy link
Member

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.

@trey-stafford @mfisher87 if either of you are able to separate out the changes in the PR that rely on #605 (maybe we can add those directly to #605?) from those that don't, it would be super to get this merged in!

@mfisher87
Copy link
Member Author

I might be able to take a look in a couple weeks, but can't promise at the moment!

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.

4 participants