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

Refactor kube-derive using darling #435

Merged
merged 4 commits into from Feb 24, 2021

Conversation

kazk
Copy link
Member

@kazk kazk commented Feb 24, 2021

Make parsing macro inputs declarative with darling.

See the diff (only the first commit that refactors) ignoring whitespace.

@kazk kazk force-pushed the refactor-derive-with-darling branch from a6a739d to 55f0a7d Compare February 24, 2021 10:44
@kazk kazk force-pushed the refactor-derive-with-darling branch from 55f0a7d to 0209892 Compare February 24, 2021 10:49
status: Option<String>,
#[darling(multiple, rename = "shortname")]
shortnames: Vec<String>,
Copy link
Member Author

Choose a reason for hiding this comment

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

You'll get a nice error like this:
image

Copy link
Member

Choose a reason for hiding this comment

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

Oo, that's nice. Assume this is using rust-analyzer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it was more that RA sometimes has issues with certain derive macros for me. But if this works fine then that's great.

kube-derive/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Beautiful. kube-derive is basically business logic only now.


/// Values we can parse from #[kube(attrs)]
#[derive(Debug, Default)]
#[derive(Debug, Default, FromDeriveInput)]
#[darling(attributes(kube))]
struct KubeAttrs {
Copy link
Member

Choose a reason for hiding this comment

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

This is awesome. Gets rid of so much code, and darling seems pretty light on dependencies as well. Great clean-up.

@clux
Copy link
Member

clux commented Feb 24, 2021

kind ci is a false negative. I am happy to merge.

@kazk
Copy link
Member Author

kazk commented Feb 24, 2021

I started looking into better tests (#179) and have some nice tests for failures using https://github.com/dtolnay/trybuild. I'll push it soon.

@@ -0,0 +1,23 @@
error: Missing field `group`
Copy link
Member Author

Choose a reason for hiding this comment

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

Shows errors from all the missing required attributes (not stopping at group anymore)

@@ -0,0 +1,5 @@
error: Enums or Unions can not #[derive(CustomResource)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Noticed the error message was missing ] thanks to this test.

@kazk
Copy link
Member Author

kazk commented Feb 24, 2021

All green now 🎉 Please merge if the new tests looks good.

@kazk kazk marked this pull request as draft February 24, 2021 20:55
@kazk
Copy link
Member Author

kazk commented Feb 24, 2021

I noticed a typo in a test file name...

@clux
Copy link
Member

clux commented Feb 24, 2021

hah, was just about to hit the button

@kazk kazk force-pushed the refactor-derive-with-darling branch from 1f59214 to 21d6754 Compare February 24, 2021 20:57
@kazk kazk marked this pull request as ready for review February 24, 2021 20:57
// If you make a change, remove `tests/ui/*.stderr` and run `cargo test`.
// Then copy the files that appear under `wip/` if it's what you expected.
// Alternatively, run `TRYBUILD=overwrite cargo test`.
// See https://github.com/dtolnay/trybuild
Copy link
Member

Choose a reason for hiding this comment

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

perfect!

@clux
Copy link
Member

clux commented Feb 24, 2021

Same false negative, will try to fix that in #437. Happy to merge.

@clux clux merged commit c7e0b97 into kube-rs:master Feb 24, 2021
@kazk kazk deleted the refactor-derive-with-darling branch February 24, 2021 21:10
@clux clux linked an issue Feb 24, 2021 that may be closed by this pull request
@clux
Copy link
Member

clux commented Feb 28, 2021

Released in 0.51.0 :-)

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.

write better tests for kube-derive
3 participants