-
Notifications
You must be signed in to change notification settings - Fork 107
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
Partially typecheck APIformatting
module
#598
Conversation
👈 Launch a binder notebook on this branch for commit 79fccf1 I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit ca3f553 👈 Launch a binder notebook on this branch for commit 3da94ef 👈 Launch a binder notebook on this branch for commit ea4a0c5 👈 Launch a binder notebook on this branch for commit b90a10e |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #598 +/- ##
===============================================
- Coverage 65.66% 65.47% -0.19%
===============================================
Files 38 38
Lines 3122 3134 +12
Branches 599 601 +2
===============================================
+ Hits 2050 2052 +2
- Misses 984 991 +7
- Partials 88 91 +3 ☔ View full report in Codecov by Sentry. |
assert key in [ | ||
"time", | ||
"temporal", | ||
], "An invalid time key was submitted for formatting." |
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.
This logic (is key
"time"
or "temporal"
?) is repeated just below, so I moved the error statement into that conditional.
In general, assertions should not be used in "the code" (use only in the tests) unless you specifically want the assertions to be controllable at runtime. python -O
will disable all assertions like -W
disables warnings. So when I moved this logic I also changed it to raise a RuntimeError
.
79fccf1
to
ca3f553
Compare
@@ -282,31 +283,14 @@ def __init__( | |||
self._fmted_keys = values if values is not None else {} | |||
|
|||
@property | |||
def poss_keys(self): | |||
def poss_keys(self) -> dict[str, list[str]]: |
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.
This property returns static data, from what I can tell, so I simplified it down, and removed _poss_keys
attribute.
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.
I think I had it this way because (1) I'd learned about properties and was probably overusing them; (2) I wanted it to be private enough the user understood they could look but shouldn't touch (so an invalid parameter wasn't submitted). Overall a fan of the simplification though.
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.
(1) I'd learned about properties and was probably overusing them;
Who can blame you, properties are awesome!
(2) I wanted it to be private enough the user understood they could look but shouldn't touch
Do you mean "can't set it" by "shouldn't touch"?
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.
Do you mean "can't set it" by "shouldn't touch"?
I suppose so.
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.
Gotcha. That's already covered by the @property
decorator :)
>>> class Foo:
... @property
... def foo(self):
... return "foo"
...
>>> Foo().foo
'foo'
>>> Foo().foo = "bar"
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: can't set attribute 'foo'
18bd814
to
a8996d1
Compare
ca3f553
to
3da94ef
Compare
Integration tests: https://github.com/icesat2py/icepyx/actions/runs/11041992788 |
Not passing. #598 fixes bug introduced on |
In the interest of time, I'm going to merge this PR with failing integration tests, having tested the solution locally and gotten this branch to pass. This will simplify #616 into a one-step change and reduce the amount of review cycles. Apologies if this is objectionable! Please let me know if so and I will avoid a repeat in the future. |
Add type annotations and some refactors to the APIformatting module.