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

Document every public derived fn from kube-derive #919

Merged
merged 4 commits into from
May 25, 2022
Merged

Conversation

clux
Copy link
Member

@clux clux commented May 24, 2022

Fixes deny(missing_docs) issues interacting with kube-derive reported in #918 cc @olix0r

Fixes deny(missing_docs) issues interacting with kube-derive reported in

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added the changelog-fix changelog fix category for prs label May 24, 2022
@clux clux added this to the 0.74.0 milestone May 24, 2022
Signed-off-by: clux <sszynrae@gmail.com>
#visibility metadata: #k8s_openapi::apimachinery::pkg::apis::meta::v1::ObjectMeta,
/// Spec on derived type
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this is actually annoying... it means schemars will use this rather than what's in front of FooSpec:

    expected:
        "Spec on derived type"
    actual:
        "Our spec for Foo\n\nA struct with our chosen Kind will be created for us, using the following kube attrs"

via crd_derive tests. Probably need to try to re-use the docs for the type in here.

Copy link
Member Author

@clux clux May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have changed the dummy doc strings for a #[allow(missing_docs)] on the generated root struct instead...

this allows the schemas to pick up the right property, although it is slightly on the hackier side

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Document every public derived fn/property via kube-derive Document every public derived fn from kube-derive May 24, 2022
@clux clux requested a review from a team May 24, 2022 18:15
@codecov-commenter

This comment was marked as off-topic.

Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to wonder whether we should just let users define their own root struct instead, but that seems like a separate issue...

@clux
Copy link
Member Author

clux commented May 25, 2022

hm, yeah, maybe. it's not something people have ever asked for, but if there's a legit case for it, we can probably consider adding an opt-out.

@clux clux merged commit 80272c0 into master May 25, 2022
@clux clux deleted the derive-full-docs branch May 25, 2022 14:14
@nightkr
Copy link
Member

nightkr commented May 25, 2022

Well one downside of the current approach is that we export CRD YAMLs with the auto-generated "blah blah auto-generated wrapper type for blahblahspec", which is only intended for the Rust-side devs.

We can work around that, but it feels like it might be time to reconsider the more special cases we end up adding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants