-
Notifications
You must be signed in to change notification settings - Fork 119
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
feat: remove pebble enum | str unions #1400
base: main
Are you sure you want to change the base?
feat: remove pebble enum | str unions #1400
Conversation
raise ValueError( | ||
'self.current must be a ServiceStatus member (e.g. ServiceStatus.ACTIVE)' | ||
f', not {self.current}' | ||
) | ||
return self.current == ServiceStatus.ACTIVE |
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 comparison would be silently False
if self.current
was accidentally a 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.
Would a better solution be to switch to using enum.StrEnum
instead of enum.Enum
? This would seem to match the intent of the original code better. With StrEnum
, ServiceStatus.ACTIVE == 'active'
, so passing around strings instead of enum members would silently succeed instead of silently failing in the absence of a type check.
self.level = CheckLevel.UNKNOWN | ||
# | ||
# these are all Optional in CheckDict and here, why '' instead of None? | ||
# note that all falsey values are filtered out in to_dict | ||
self.period: Optional[str] = dct.get('period', '') | ||
self.timeout: Optional[str] = dct.get('timeout', '') | ||
self.threshold: Optional[int] = dct.get('threshold') |
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.
In general a lot of values are marked as Optional
when they could perhaps be simply left out of the TypedDict
. I haven't fully checked what the values received from Pebble for these dicts can be -- can these items be the go
version of None
(null
? nil
?). It seems like if so, we should use None
instead of ''
, and if not we shouldn't use Optional
.
# so do we want to just use 'unknown' for CheckLevel.UNKNOWN here? | ||
# we still get a warning on making `c1` but not on `c2`, but maybe that's ok | ||
# but is it useful to have 'unknown' in the dict? | ||
# It'll error if that value makes it to pebble, right? | ||
fields = [ |
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'm not really sure what behaviour we want here with CheckLevel.UNKNOWN
.
# while we might want <some-unknown-value-string> to overwrite, | ||
# would we ever want Checklevel.UNKNOWN to overwrite? | ||
if value is CheckLevel.UNKNOWN: | ||
continue |
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 we should just use other.to_dict()
here, is there a reason we weren't already?
@@ -3046,6 +3105,9 @@ def get_checks( | |||
""" | |||
query: Dict[str, Any] = {} | |||
if level is not None: | |||
# is this bad now that value could be 'unknown'? | |||
# should we explicitly avoid it here? | |||
# if level is not None and level.value is not CheckLevel.UNKNOWN: | |||
query['level'] = level.value |
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.
Another case where it's not clear how to handle CheckLevel.UNKNOWN
on the outgoing side.
@@ -3098,7 +3160,7 @@ def get_notices( | |||
*, | |||
users: Optional[NoticesUsers] = None, | |||
user_id: Optional[int] = None, | |||
types: Optional[Iterable[Union[NoticeType, str]]] = None, | |||
types: Optional[Iterable[NoticeType]] = None, | |||
keys: Optional[Iterable[str]] = None, | |||
) -> List[Notice]: | |||
"""Query for notices that match all of the provided filters. |
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 wonder if this will break any users.
assert info.type == 'foobar' | ||
with pytest.warns(UserWarning): | ||
info = pebble.FileInfo.from_dict(d) | ||
assert info.type == pebble.FileType.UNKNOWN |
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.
Should we be using a different kind of warning here?
@@ -3863,7 +3863,7 @@ def _notice_matches( | |||
# For example: if user_id filter is set and it doesn't match, return False. | |||
if user_id is not None and not (notice.user_id is None or user_id == notice.user_id): | |||
return False | |||
if types is not None and notice.type not in types: | |||
if types is not None and notice.type.value not in types: | |||
return False |
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.
types
is a collection of str
s. I think this is mimicking the logic of pebble
, so I think it makes sense for us to use notice.type.value
here. Was this always silently broken? Or silently worked because notice.type
was often a str
-- while now it's supposed to always be an enum
member?
Resolving #1287, this PR removes the use of
str
andenum
member unions connected to thepebble
module.Generally this just involves typing
dict
s generated by pebble as containingstr
values, and using theenum
types exclusively inops
. The major change is that unknown values from pebble result in anUNKNOWN
member of theenum
, rather than being passed around asstr
s.Reviewers will notice some block comments calling attention to potential issues with the current code, questions about my design choices, and suggestions for further changes. Feedback on these areas would be especially appreciated.