Rewrite AsChangeset using syn 0.12
#1512
Conversation
|
/cc @dtolnay mind taking a look at this? |
| } | ||
| } | ||
|
|
||
| pub fn camel_to_snake(name: &str) -> String { |
There was a problem hiding this comment.
This function and infer_table_name were both copied over from the old codebase. I think I want to deprecate the inferred table name behavior (I want to hold off on that until everything is moved over though)
sgrif
force-pushed
the
sg-rewrite-as-changeset
branch
2 times, most recently
from
21e8811
to
10ee483
Compare
In order to update to syn 0.12, we're going to have to rewrite a lot of `diesel_derives`. A lot of code is incompatible with no obvious migration path. I also want to take this as an opportunity to do a lot of long needed refactoring to this library. Despite continuous assertions that outputting `compile_error!` somehow makes our error messages significantly better (it really doesn't, it just avoids "proc-macro panicked") -- I am panicking in error cases here. One of the benefits of syn 0.12 is that it retains span information, which we can use on nightly to output errors with real diagnostics. I haven't made that jump here quite yet, but I plan to in a followup PR. Since this is the first derive moved over to the new crate, it includes much more code than PRs for the rest will.
sgrif
force-pushed
the
sg-rewrite-as-changeset
branch
from
10ee483
to
72614e7
Compare
|
After playing around with |
|
I guess I'll restructure to not-panic separately.. |
diesel_derives2/src/model.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn table_name(&self) -> Cow<syn::Ident> { |
There was a problem hiding this comment.
Ident is cheaply copyable so I would not bother with Cow here.
pub fn table_name(&self) -> syn::Ident {
self.table_name_from_attribute
.unwrap_or_else(|| infer_table_name(self.name))
}
diesel_derives2/src/model.rs
Outdated
| .unwrap_or_else(|_| vec!["id".into()]); | ||
| let fields = fields_from_item_data(&item.data); | ||
| Self { | ||
| name: item.ident.clone(), |
There was a problem hiding this comment.
This can be item.ident without clone().
diesel_derives2/src/field.rs
Outdated
| .ok() | ||
| .map(|m| m.expect_ident_value()); | ||
| let identifier = match field.ident { | ||
| Some(ref x) => Identifier::Named(x.clone()), |
|
|
||
| let (_, ty_generics, where_clause) = item.generics.split_for_impl(); | ||
| let mut impl_generics = item.generics.clone(); | ||
| impl_generics.params.push(parse_quote!('update)); |
There was a problem hiding this comment.
You are going to want another split_for_impl after this line.
let (impl_generics, _, _) = impl_generics.split_for_impl();Otherwise it does not work for types that have generic type parameter defaults.
#[derive(AsChangeset)]
#[table_name = "users"]
#[primary_key(name)]
struct Changes2<T = String> {
name: T,
hair_color: String,
}error: defaults for type parameters are only allowed in `struct`, `enum`, `type`, or `trait` definitions.
--> tests/update.rs:390:14
|
390 | #[derive(AsChangeset)]
| ^^^^^^^^^^^
|
= note: #[deny(invalid_type_param_default)] on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #36887 <https://github.com/rust-lang/rust/issues/36887>
There was a problem hiding this comment.
This code doesn't currently work with generic structs, but good catch.
| repository = "https://github.com/diesel-rs/diesel/tree/master/diesel_derives" | ||
|
|
||
| [dependencies] | ||
| syn = { version = "0.12.0", features = ["full"] } |
There was a problem hiding this comment.
Do we really need the full feature here?
As far as I can see the default features should be sufficient for diesel.
There was a problem hiding this comment.
Our test suite fails without the full feature.
| let column_name_from_attribute = MetaItem::with_name(&field.attrs, "column_name") | ||
| .ok() | ||
| .map(|m| m.expect_ident_value()); | ||
| let identifier = match field.ident { |
There was a problem hiding this comment.
Wouldn't
field.ident.map(Identifier::Named).unwrap_or_else(|| Identifier::Unnamed(index.to_string().into())be a bit more idiomatic?
diesel_derives2/src/field.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn column_name(&self) -> &syn::Ident { |
There was a problem hiding this comment.
Ident seems to be Copy now. Maybe return no reference than?
Also fixes a potential future issue with generic structs. Without the `.split_for_impl` line, we would fail on any type parameter with defaults (right now we don't support generic structs at all here, but this will be needed if we ever do)
In order to update to syn 0.12, we're going to have to rewrite a lot of
diesel_derives. A lot of code is incompatible with no obviousmigration path. I also want to take this as an opportunity to do a lot
of long needed refactoring to this library.
Despite continuous assertions that outputting
compile_error!somehowmakes our error messages significantly better (it really doesn't, it
just avoids "proc-macro panicked") -- I am panicking in error cases
here. One of the benefits of syn 0.12 is that it retains span
information, which we can use on nightly to output errors with real
diagnostics. I haven't made that jump here quite yet, but I plan to in a
followup PR.
Since this is the first derive moved over to the new crate, it includes
much more code than PRs for the rest will.