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

feat: add generated status dataclasses #26

Merged
merged 6 commits into from
Feb 19, 2025
Merged

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Feb 16, 2025

These come from the Go generator in benhoyt/juju#2 (which now has the is_* methods added to UnitStatus and AppStatus).

Also renames ApplicationStatus to AppStatus for consistency (we're abbreviating "application" to "app" everywhere).

Fixes #17.

These come from the Go generator in:
benhoyt/juju#2
@benhoyt
Copy link
Collaborator Author

benhoyt commented Feb 16, 2025

@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.

Copy link

@james-garner-canonical james-garner-canonical left a 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

Copy link

@tonyandrewmeyer tonyandrewmeyer left a 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.

@benhoyt
Copy link
Collaborator Author

benhoyt commented Feb 18, 2025

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.

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.

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?

The plan is to add docstrings "as needed". I've added a basic docstring to the top-level Status one for now. Unfortunately none of the fields have doc comments on the Go side, otherwise we could use those. So this isn't really documented anywhere. I suspect most people will find what they're looking for with the decent typing information, but we'll see.

@benhoyt benhoyt merged commit d2d1daf into canonical:main Feb 19, 2025
@benhoyt benhoyt deleted the types branch February 19, 2025 03:39
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.

Flesh out status types in _types.py
3 participants