-
Notifications
You must be signed in to change notification settings - Fork 12
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
API: Wrap *Kind
enums in *Kind
enums
#244
Conversation
marker_api/src/ast/stmt.rs
Outdated
#[cfg(feature = "driver-api")] | ||
impl<'ast> ItemStmt<'ast> { | ||
pub fn new(data: CommonStmtData<'ast>, item: ItemKind<'ast>) -> Self { | ||
Self { data, item } | ||
} | ||
} |
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.
Could it make sense to just mark the fields of the structs conditionally pub
instead?
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.
The visibility
crate sadly doesn't work on field structs. Otherwise, that could be an option. I considered adding something like impl_new
to cfg derive these things. I was a bit hesitant with Marker's FFI types, but I just saw that it uses impl Into<Ty>
that could actually work 🤔
Edit: https://docs.rs/derive-new/0.5.9/derive_new/ is probably the safer choice, since impl_new
is very new. Let's hope that one also has the Into<>
thing 😅
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 wouldn't add a proc macro dependency to marker_api
. I love the fact that it can be a zero dependency crate, meaning the lint crates can also be effectively zero-dep (except for marker_api
), and compile blazingly fast
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.
The macro would only be needed when it's compiled with the driver-api
feature, so it would remain zero-dep :D
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, right!
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'm undecided on this one. impl_new looks great but only has a total of 102 downloads. derive-new is from @nrc
but doesn't seem to support Into<X>
parameters, which is important for marker. It also doesn't seem like the crate isn't actively maintained, when I look at the issue tracker 🤔
Created: nrc/derive-new#62
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'd recommend you to use either typed-builder
, for more elaborate cases: buildstructor
.
typed-builder
has support for impl Into
via #[builder(setter(into))]
(opt-in), and buildstructor
just slaps impl Into
automatically (opt-out).
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'll try using typed-builder
instead of the newly added constructors, wish me luck!
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.
It seems to work quite well. I won't update everything rn, but I'll keep the dependency and use it for new types :)
Here are the changes: 367cfb5
Looks like Clippy doesn't like it though :/ I'll allow the lint and create an issue
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.
There was also another lint that caused problems clippy::used_underscore_binding
(idanarye/rust-typed-builder#113)
Let's hope everything works now :)
Co-authored-by: Veetaha <gersoh3@gmail.com>
10c1c0c
to
367cfb5
Compare
367cfb5
to
84b4897
Compare
84b4897
to
3ed653e
Compare
Thank you for the review @Veetaha, I highly appreciate it ❤️ |
The current API has a few spots where a
*Kind
enum directly wraps a*Kind
enum, like:PatKind::Lit
. This is cool, for matching but causes a bunch of problems when it comes to common data shared among the outer*Kind
enum. To be future-proof and have this consistent in the entire API, I've now wrapped the inner*Kind
values in new structs.These structs are often just wrappers, but will help us in the future. This will help me a lot with #242
Most of the changes are just uilint tests.