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
762 changes: 444 additions & 318 deletions doc/source/user_guide/documentation/classes_dev_uml.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
560 changes: 344 additions & 216 deletions doc/source/user_guide/documentation/classes_user_uml.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
188 changes: 106 additions & 82 deletions doc/source/user_guide/documentation/packages_user_uml.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
79 changes: 43 additions & 36 deletions icepyx/core/APIformatting.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
"""Generate and format information for submitting to API (CMR and NSIDC)."""

import datetime as dt
from typing import Any, Generic, Literal, TypeVar, Union, overload
from typing import Any, Generic, Literal, Optional, TypeVar, Union, overload

from icepyx.core.exceptions import ExhaustiveTypeGuardException, TypeGuardException
from icepyx.core.types import (
CMRParams,
EGIParamsSubset,
Expand Down Expand Up @@ -36,10 +37,6 @@

assert isinstance(start, dt.datetime)
assert isinstance(end, dt.datetime)
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.


if key == "temporal":
fmt_timerange = (
Expand All @@ -53,6 +50,8 @@
+ ","
+ end.strftime("%Y-%m-%dT%H:%M:%S")
)
else:
raise ValueError("An invalid time key was submitted for formatting.")

Check warning on line 54 in icepyx/core/APIformatting.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/APIformatting.py#L54

Added line #L54 was not covered by tests

return {key: fmt_timerange}

Expand Down Expand Up @@ -231,7 +230,7 @@
Returns the dictionary of formatted keys associated with the
parameter object.
"""
return instance._fmted_keys
return instance._fmted_keys # pyright: ignore[reportReturnType]


# ----------------------------------------------------------------------
Expand All @@ -257,13 +256,16 @@
on the type of query. Must be one of ['search','download']
"""

partype: T
_reqtype: Optional[Literal["search", "download"]]
fmted_keys = _FmtedKeysDescriptor()
# _fmted_keys: Union[CMRParams, EGISpecificRequiredParams, EGIParamsSubset]

def __init__(
self,
partype: T,
values=None,
reqtype=None,
values: Optional[dict] = None,
reqtype: Optional[Literal["search", "download"]] = None,
):
assert partype in [
"CMR",
Expand All @@ -282,31 +284,14 @@
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'

"""
Returns a list of possible input keys for the given parameter object.
Possible input keys depend on the parameter type (partype).
"""

if not hasattr(self, "_poss_keys"):
self._get_possible_keys()

return self._poss_keys

# @property
# def wanted_keys(self):
# if not hasattr(_wanted):
# self._wanted = []

# return self._wanted

def _get_possible_keys(self) -> dict[str, list[str]]:
"""
Use the parameter type to get a list of possible parameter keys.
"""

if self.partype == "CMR":
self._poss_keys = {
return {
"spatial": ["bounding_box", "polygon"],
"optional": [
"temporal",
Expand All @@ -316,7 +301,7 @@
],
}
elif self.partype == "required":
self._poss_keys = {
return {
"search": ["short_name", "version", "page_size"],
"download": [
"short_name",
Expand All @@ -331,7 +316,7 @@
],
}
elif self.partype == "subset":
self._poss_keys = {
return {
"spatial": ["bbox", "Boundingshape"],
"optional": [
"time",
Expand All @@ -341,8 +326,17 @@
"Coverage",
],
}
else:
raise ExhaustiveTypeGuardException

Check warning on line 330 in icepyx/core/APIformatting.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/APIformatting.py#L330

Added line #L330 was not covered by tests

# @property
# def wanted_keys(self):
# if not hasattr(_wanted):
# self._wanted = []

def _check_valid_keys(self):
# return self._wanted

def _check_valid_keys(self) -> None:
"""
Checks that any keys passed in with values are valid keys.
"""
Expand All @@ -352,13 +346,13 @@

val_list = list({val for lis in self.poss_keys.values() for val in lis})

for key in self.fmted_keys:
for key in self.fmted_keys: # pyright: ignore[reportAttributeAccessIssue]
assert key in val_list, (
"An invalid key (" + key + ") was passed. Please remove it using `del`"
)

# DevNote: can check_req_values and check_values be combined?
def check_req_values(self):
def check_req_values(self) -> bool:
"""
Check that all of the required keys have values, if the key was passed in with
the values parameter.
Expand All @@ -367,17 +361,22 @@
assert (
self.partype == "required"
), "You cannot call this function for your parameter type"

if not self._reqtype:
raise TypeGuardException

Check warning on line 366 in icepyx/core/APIformatting.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/APIformatting.py#L366

Added line #L366 was not covered by tests

reqkeys = self.poss_keys[self._reqtype]

if all(keys in self.fmted_keys for keys in reqkeys):
if all(keys in self.fmted_keys for keys in reqkeys): # pyright: ignore[reportAttributeAccessIssue]
assert all(
self.fmted_keys.get(key, -9999) != -9999 for key in reqkeys
self.fmted_keys.get(key, -9999) != -9999 # pyright: ignore[reportAttributeAccessIssue]
for key in reqkeys
), "One of your formatted parameters is missing a value"
return True
else:
return False

def check_values(self):
def check_values(self) -> bool:
"""
Check that the non-required keys have values, if the key was
passed in with the values parameter.
Expand All @@ -391,7 +390,8 @@
# not the most robust check, but better than nothing...
if any(keys in self._fmted_keys for keys in spatial_keys):
assert any(
self.fmted_keys.get(key, -9999) != -9999 for key in spatial_keys
self.fmted_keys.get(key, -9999) != -9999 # pyright: ignore[reportAttributeAccessIssue]
for key in spatial_keys
), "One of your formatted parameters is missing a value"
return True
else:
Expand Down Expand Up @@ -427,6 +427,9 @@
self._check_valid_keys()

if self.partype == "required":
if not self._reqtype:
raise TypeGuardException

Check warning on line 431 in icepyx/core/APIformatting.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/APIformatting.py#L431

Added line #L431 was not covered by tests

if self.check_req_values() and kwargs == {}:
pass
else:
Expand Down Expand Up @@ -484,6 +487,7 @@
if any(keys in self._fmted_keys for keys in spatial_keys):
pass
else:
k = None
if self.partype == "CMR":
k = kwargs["extent_type"]
elif self.partype == "subset":
Expand All @@ -492,6 +496,9 @@
elif kwargs["extent_type"] == "polygon":
k = "Boundingshape"

if not k:
raise TypeGuardException

Check warning on line 500 in icepyx/core/APIformatting.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/APIformatting.py#L500

Added line #L500 was not covered by tests

self._fmted_keys.update({k: kwargs["spatial_extent"]})


Expand Down
29 changes: 29 additions & 0 deletions icepyx/core/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
ISSUE_REPORTING_INSTRUCTIONS = (
"If you are a user seeing this message, the developers of this software have made a"
" mistake! Please report the full error traceback in the icepyx GitHub repository:"
" <https://github.com/icesat2py/icepyx/issues/new>"
)


class DeprecationError(Exception):
"""
Class raised for use of functionality that is no longer supported by icepyx.
Expand All @@ -24,3 +31,25 @@

def __str__(self):
return f"{self.msgtxt}: {self.errmsg}"


class TypeGuardException(Exception):
"""
Should never be raised at runtime.

Used in cases where a runtime check is not desired, but we want to add a "type guard"
(https://github.com/microsoft/pyright/blob/main/docs/type-concepts-advanced.md#type-guards)
to give the type checker more information.
"""

def __str__(self):
return ISSUE_REPORTING_INSTRUCTIONS

Check warning on line 46 in icepyx/core/exceptions.py

View check run for this annotation

Codecov / codecov/patch

icepyx/core/exceptions.py#L46

Added line #L46 was not covered by tests


class ExhaustiveTypeGuardException(TypeGuardException):
"""
Should never be raised at runtime.

Used exclusively in cases where the typechecker needs a typeguard to tell it that a
check is exhaustive.
"""
1 change: 0 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ exclude = [
# DevGoal: Remove all ignores
ignore = [
"icepyx/quest/*",
"icepyx/core/APIformatting.py",
"icepyx/core/auth.py",
"icepyx/core/is2ref.py",
"icepyx/core/read.py",
Expand Down
Loading