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

Partially typecheck APIformatting module #598

Merged
merged 10 commits into from
Sep 25, 2024

Conversation

mfisher87
Copy link
Member

@mfisher87 mfisher87 commented Sep 5, 2024

Add type annotations and some refactors to the APIformatting module.

Copy link

github-actions bot commented Sep 5, 2024

Binder 👈 Launch a binder notebook on this branch for commit 79fccf1

I will automatically update this comment whenever this PR is modified

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

Binder 👈 Launch a binder notebook on this branch for commit 3da94ef

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

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

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 60.71429% with 11 lines in your changes missing coverage. Please review.

Project coverage is 65.47%. Comparing base (1606dd0) to head (b90a10e).
Report is 11 commits behind head on development.

Files with missing lines Patch % Lines
icepyx/core/APIformatting.py 56.52% 5 Missing and 5 partials ⚠️
icepyx/core/exceptions.py 80.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

assert key in [
"time",
"temporal",
], "An invalid time key was submitted for formatting."
Copy link
Member Author

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.

@mfisher87 mfisher87 force-pushed the typecheck-apiformatting branch from 79fccf1 to ca3f553 Compare September 5, 2024 15:15
@@ -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]]:
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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"?

Copy link
Member

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.

Copy link
Member Author

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'

@mfisher87 mfisher87 force-pushed the refactor-and-typecheck-api-interfaces branch from 18bd814 to a8996d1 Compare September 12, 2024 16:22
Base automatically changed from refactor-and-typecheck-api-interfaces to development September 17, 2024 23:48
@mfisher87 mfisher87 force-pushed the typecheck-apiformatting branch from ca3f553 to 3da94ef Compare September 18, 2024 00:12
@mfisher87
Copy link
Member Author

@mfisher87
Copy link
Member Author

Not passing. #598 fixes bug introduced on development branch from my last-merged PR.

@mfisher87
Copy link
Member Author

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.

@mfisher87 mfisher87 merged commit 0f2cfeb into development Sep 25, 2024
7 of 10 checks passed
@mfisher87 mfisher87 deleted the typecheck-apiformatting branch September 25, 2024 23:51
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.

2 participants