-
Notifications
You must be signed in to change notification settings - Fork 113
A collection of "low-hanging fruit" refactors #612
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
base: typecheck-spatial
Are you sure you want to change the base?
Conversation
|
I will automatically update this comment whenever this PR is modified
|
| if not hasattr(self, "_granules") or self._granules is None: | ||
| self._granules = Granules() | ||
|
|
||
| return self._granules |
There was a problem hiding this comment.
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.
91bd9a8 to
b75d000
Compare
a63ae13 to
bfa9562
Compare
b75d000 to
2e7ac19
Compare
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
Looks like this PR is ready-to-go pending #605 |
Co-authored-by: Jessica Scheick <[email protected]>
Any idea why this shouldn't go directly into development? |
@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! |
|
I might be able to take a look in a couple weeks, but can't promise at the moment! |
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!