-
Notifications
You must be signed in to change notification settings - Fork 3
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: add generated status dataclasses #26
Conversation
These come from the Go generator in: benhoyt/juju#2
@tonyandrewmeyer and @james-garner-canonical Not sure the best way to review. Probably no need to review line by line, as this is all generated code, but maybe do a spot check of the different field types. You could also scan over the code generator in benhoyt/juju#2. |
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.
Ah few thoughts. I had a look at the codegen but not particularly deeply. If I haven't commented specifically on something here I likely haven't thought about it
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.
Wow, this really is a lot. I definitely agree that hiding most of this away under jubilant.types
or similar is a good idea.
It feels like it would be nice if it was more selective in the way that the calls only expose the most used methods/arguments and fall back to juju.cli
for the rest, but dropping down to "and then the rest is a dict[str, any]" doesn't seem ideal. I guess being generated from the Go makes it worthwhile.
What's the plan with documenting these? Some of them are probably obvious enough if you know the Juju CLI, but it is nice if there's an actual docstring - but I assume you wouldn't want to notate every class and attribute by hand. If you add docstrings does that make it hard to re-generate?
I spot checked a couple, but didn't look at the Go generator. I'm trusting that's working correctly (or would otherwise be clearly wrong). I'm also assuming they're all needed - I didn't work through the chain to see that everything is linked to status.
This has been updated in the generator: benhoyt/juju@0a421ce Also lowercase strange "FilesystemInfo.Attachments" field name.
Yeah, that's my take too. I probably wouldn't have done all these if doing them by hand, but with generating them it seems reasonable.
The plan is to add docstrings "as needed". I've added a basic docstring to the top-level |
These come from the Go generator in benhoyt/juju#2 (which now has the
is_*
methods added toUnitStatus
andAppStatus
).Also renames
ApplicationStatus
toAppStatus
for consistency (we're abbreviating "application" to "app" everywhere).Fixes #17.