-
Notifications
You must be signed in to change notification settings - Fork 126
fix: minor hookcmds fixes #2175
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
fix: minor hookcmds fixes #2175
Conversation
| unit: The unit to get data for, or ``None`` to get data for the unit | ||
| that triggered the current hook. |
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 believe None will get data for the unit the charm code is running on, not necessarily the unit that triggered the current event
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.
nvm I was mistaken
(think I got mixed up by units having the same data in the databag)
dimaqq
left a comment
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.
Let's validate Carl's comment in real juju.
| result = cast('dict[str, Any]', json.loads(stdout)) if key is None else json.loads(stdout) | ||
| return result |
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.
Sorry to revisit a discussion we've had before, but I really don't think the 'cast from json.loads then return' pattern is beneficial here:
- We're now returning an
Anytyped object in one branch (the result ofjson.loads(stdout)). - All other types, including
dict[str, Any]are logically subtypes of it. - The previous
castto two disjoint types was unnecessary due to the immediate return, but casting to a subtype feels somehow even more unnecessary.
Additionally:
- The overloads already capture the return type logic of this function clearly and concisely, and that's all the user will see.
- Most importantly, the conditional cast complexity for readers and maintainers to an otherwise very short and clear function -- for example the changes in this PR are really only to the return type annotation, but require touching the internal
if ... elseexpression too without making any actual runtime changes.
| result = cast('dict[str, Any]', json.loads(stdout)) if key is None else json.loads(stdout) | |
| return result | |
| return json.loads(stdout) |
If we're really committed to using cast internally to re-document return types, how about lifting the actual evaluation of the result out of the cast logic:
result = json.loads(stdout)
if key is None:
return cast('dict[str, Any]', result)
return resultThere 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.
Sorry to revisit a discussion we've had before
I do feel like this is shoe-horning in a previously-resolved discussion into a PR that is mostly unrelated.
I'm ok with re-opening this discussion, but I'd rather that was done through opening an issue or PR that addresses this throughout all of hookcmds (or alternatively, adding to the Charm Tech daily discussions).
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.
My rationale for revisiting it here is that even if we think it's the right choice everywhere else in hookcmds I think the changes introduced in this PR make it the wrong choice here, and I'd prefer it not to be committed as-is. But I approved because I do think it's valid call to want to keep it this way for consistency and a smaller diff.
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.
Yes, which is why I'm leaving it in place. One of the primary reasons that hookcmds is like this is because there was a desire / request to have all of the methods consistent in this area, so I don't want to break that consistency in this PR.
Small fixes for the
hookcmdspackage:modelversion unchanged, but that's more specifically expecting a dict)action-get, which can be any action parameter type, which means essentiallyAnyrelation-get'sunitargumentThese were reported by @carlcsaposs-canonical , thanks!