Improve derive syntax #2626

pksunkara · 2021-01-19T19:49:49Z

weiznich

That looks quite good 👍 Left a first bunch of comments. There are likely a few more things which need to change later on or I've missed now.

.vscode/settings.json

diesel_derives/src/attrs.rs

diesel_derives/src/field.rs

diesel_derives/src/insertable.rs

diesel_derives/src/lib.rs

diesel_derives/src/model.rs

pksunkara · 2021-01-20T12:36:26Z

Still some work to do. I just wanted to create a draft PR to show some progress. You can see that the commits are from 2 months ago :)

Regarding duplicated attributes, neither clap nor other repos which follow this overridable pattern has warnings. And I actually think it's good like that because it might open up some modularized use cases for end users.

weiznich · 2021-05-31T12:57:37Z

@pksunkara Are you interested in finishing this PR in the next few weeks?

pksunkara · 2021-05-31T13:01:57Z

Yup, I was just rebasing this.

pksunkara · 2021-06-30T11:07:11Z

@weiznich Why does selectable derive have sql_type attribute helper? reference. I don't see it being used anywhere in the selectable expansion.

Also, since this is a new derive, we don't need backward compatibility for this and remove the old attribute helpers, right?

weiznich · 2021-06-30T12:02:06Z

Yes, sql_type is not used there.

Also, since this is a new derive, we don't need backward compatibility for this and remove the old attribute helpers, right?

Sounds reasonable 👍

pksunkara · 2021-07-07T02:55:30Z

@weiznich Can you please give a preliminary look and see if you are okay with the format of the new attributes? Field attribute and Struct attribute

pksunkara · 2021-07-07T10:28:48Z

CI is weird. I am locally on 1.48.0 and am unable to reproduce this error at all. Or the spanned::Spanned error. ./bin/test is passing completely on my local.

weiznich

I've done another round of review and left some comments on places where I have questions or small suggestions.

General points to discuss:

As far as I see there is currently no way to emit a deprecation warning on a build not using rustc nightly. That's bad, we should find some way to "fix" that, even if that means to emit some "warnings" via eprintln!
The get_parent() error for the minimal rust version build is caused by -Z minimal-versions, which just means that the minimal syn version that is specified do not have the corresponding method. This should be fixed by just increasing the minimal supported syn version to some version that has this method (at least syn 1.0.3 as far as I can see)
There are a few places the old deprecated attributes are not converted. The CI mostly did emit warnings for them, they should be fixed
I'm not sure about the spanned::Spanned error, are you really sure that this trait is really needed in that scope for all backends?

diesel/src/associations/mod.rs

diesel/src/mysql/types/mod.rs

diesel/src/type_impls/json.rs

diesel_compile_tests/tests/fail/as_changeset_bad_column_name_syntax.stderr

diesel_compile_tests/tests/fail/as_changeset_bad_primary_key_syntax.stderr

diesel_derives/src/model.rs

diesel_derives/src/queryable_by_name.rs

diesel_derives/src/util.rs

diesel_derives/tests/queryable_by_name.rs

diesel_tests/tests/schema/mod.rs

pksunkara · 2021-07-13T00:30:07Z

I haven't gone through the error/warning messages yet for the compile tests. I should have specifically said not to review those files inorder to save some time.

weiznich · 2021-07-13T17:20:52Z

As far as I see there is currently no way to emit a deprecation warning on a build not using rustc nightly. That's bad, we should find some way to "fix" that, even if that means to emit some "warnings" via eprintln!

So the Readme states:

Warnings are emitted only on nightly, they are ignored on stable.

which is unfortunate, as we really want to show the deprecated message everywhere. I see a few options around this:

Provide our own wrapping macro that emits warnings via eprintln! instead of just ignoring them. This likely requires a similar version check than done by proc-macro-error
Try to fix that in proc-macro-error by maybe using eprintln! there?
Emit an error instead? Maybe behind a feature flag? (Diesel already has a with/without-deprectated feature flag) (I'm not really sold on that option)

pksunkara · 2021-07-13T23:06:26Z

Yeah, I am not exactly sure what we want to do regarding that. I am leaving it for the last thing to do though. Hopefully we will come up with something by then.

pksunkara · 2021-09-22T18:42:25Z

@weiznich This is ready for review

weiznich

Thanks for the update 👍
This looks much better now. I left a few comments where I noticed small things. I think most of them should be easy to fix, some will likely require just a clarification.

diesel_compile_tests/rust-toolchain

diesel_compile_tests/tests/fail/derive/bad_aggregate.stderr

diesel_compile_tests/tests/fail/derive/bad_belongs_to.stderr

diesel_compile_tests/tests/fail/derive/selectable.rs

diesel_compile_tests/tests/fail/derive_deprecated/deprecated_belongs_to.stderr

diesel_derives/src/parsers/postgres_type.rs

diesel_derives/src/util.rs

diesel_derives/tests/queryable_by_name.rs

guide_drafts/trait_derives.md

weiznich · 2021-11-23T19:29:49Z

I should probably comment here to say that I've a rebased version of this locally, which also fixes at least some of the remarks. I can try to clean it up a bit tomorrow and push it afterwards.

pksunkara · 2021-11-23T19:31:59Z

I should probably comment here to say that I've a rebased version of this locally, which also fixes at least some of the remarks. I can try to clean it up a bit tomorrow and push it afterwards.

Oh, I was just about to do the rebase for the final push this week to get this merged. I will wait for you. Would you mind if you create a new commit for your changes instead of bundling them up with my rebased commits?

weiznich · 2021-11-23T20:04:46Z

I've pushed my rebased version. Hopefully this is a good starting point for fixing the remaining issues.

diesel/src/type_impls/primitives.rs

pksunkara · 2021-12-06T09:09:18Z

I am not sold on the commit you pushed since we have the span already at dba9212 (#2626).

I will be able to take a deeper look tomorrow and also address the last comment that's left.

weiznich · 2021-12-06T14:28:54Z

I am not sold on the commit you pushed since we have the span already at dba9212 (#2626).

I think an important point to note here is that there are a few different spans involved here:

The span of the field as whole, as returned by SynField.span(). That's the span that seems to point only to the beginning of the field.
The span of each element in the attribute. So in a #[diesel(embed, serialize_as = "…")] attribute are two of those spans. One pointing to embed and one pointing to serialize_as = "…". Those are stored in ident_span
The span of a field attribute as whole thing. For #[diesel(embed, serialize_as = "…")] this will point to diesel(embed, serialize_as = "…"). It seems like trying to point to the whole attribute just creates a single char span at the beginning 😞 . That needs to be a separate entry as there may be more than one attribute here per field. That stored in attribute_span
The span of the field itself. I opted to use either the span of the field name (for structs with named fields) or the span of the field type (for tuple structs) for this. That stored in field.span

pksunkara · 2021-12-09T10:25:19Z

I am going to separate the AttributeSpanWrapper work into a separate PR that will get opened up once this is merged as a fast follow. I think we need a little bit discussion on it. And I might have a better idea on how to do that.

I pulled the commit locally and saved it.

pksunkara · 2021-12-09T14:23:26Z

Addressed all the comments now. The only question left from my original goal of the work is:

TypePath for sql_type, serialize_as, deserialize_as?

Can you please give a look at https://docs.rs/syn/latest/syn/enum.Type.html and tell me which of those variants do we allow for the above mentioned attributes?

weiznich

Just a bunch of minor notes. Those should be easy to fix. Otherwise I think this now looks really good 🎉.

One thing is still missing: A changelog entry + probably some draft section for the upgrade guide (Just some few words that we updated the attribute format…)

diesel_compile_tests/tests/fail/derive/bad_not_sized.rs

weiznich · 2021-12-09T14:17:53Z

diesel_compile_tests/tests/fail/derive/bad_postgres_type.stderr

+error: unexpected `oid` when `name` is present
+  --> $DIR/bad_postgres_type.rs:30:44
+   |
+30 | #[diesel(postgres_type(name = "foo", oid = 2, array_oid = 3))]


As a thing for the followup PR: This spans could also be improved using the same strategy as demonstrated for deserialize_as/embed

diesel_derives/src/parsers/postgres_type.rs

diesel_derives/src/parsers/mysql_type.rs

diesel_derives/src/parsers/sqlite_type.rs

weiznich · 2021-12-09T14:53:05Z

Can you please give a look at https://docs.rs/syn/latest/syn/enum.Type.html and tell me which of those variants do we allow for the above mentioned attributes?

I would just accept TypePath there and nothing else. If that turns out to be not enough we can easily include more variants later on.

pksunkara · 2021-12-10T01:26:02Z

Address review and used TypePath

weiznich

Let's merge this 🎉

weiznich · 2021-12-10T08:25:51Z

@pksunkara I want to use this opportunity to thank your for your long term effort here. I really appreciate this. ❤️

pksunkara · 2021-12-10T13:59:14Z

No problem. I just wish I had the time to actually focus on this properly in a straight forward manner instead of doing over a long time.

pksunkara added this to the 2.0 milestone Jan 19, 2021

pksunkara linked an issue Jan 19, 2021 that may be closed by this pull request

Diesel_derives only considers flags comming from one of multiple #[diesel(…)] atttributes #2584

Closed

pksunkara marked this pull request as draft January 19, 2021 19:50

weiznich requested a review from a team January 20, 2021 07:43

weiznich requested changes Jan 20, 2021

View reviewed changes

weiznich mentioned this pull request Jan 26, 2021

Update changeset options to use a boolean value #457

Closed

pksunkara linked an issue Feb 4, 2021 that may be closed by this pull request

Update changeset options to use a boolean value #457

Closed

weiznich reviewed Jul 8, 2021

View reviewed changes

pksunkara marked this pull request as ready for review September 22, 2021 18:42

weiznich mentioned this pull request Sep 23, 2021

feat: Implement Clone for OwnedBatchInsert #2909

Closed

weiznich reviewed Sep 27, 2021

View reviewed changes

pksunkara commented Nov 23, 2021

View reviewed changes

diesel/src/type_impls/primitives.rs Outdated Show resolved Hide resolved

weiznich mentioned this pull request Nov 28, 2021

More code reviewers wanted! #1186

Open

pksunkara and others added 3 commits December 9, 2021 14:20

Update all the derive attributes to new format

91676df

Deprecate old attributes

1361676

Allow non-ident's as column names for QueryableByName

caf29cc

weiznich reviewed Dec 9, 2021

View reviewed changes

weiznich and others added 5 commits December 9, 2021 15:42

Improve the warning messages for deprecated attributes

7241ddc

Add info about allowed attributes to error

1eb2835

Add help notes for unexpected end of inputs

be698c0

Use TypePath for some attributes

fb245d6

Address review comments

8e4e7c4

weiznich approved these changes Dec 10, 2021

View reviewed changes

weiznich merged commit 6aadc7f into diesel-rs:master Dec 10, 2021

pksunkara deleted the derive branch December 10, 2021 13:55

Ten0 mentioned this pull request Dec 16, 2021

cfg_attr does not work in combination with diesel derive attribute #2983

Closed

Elrendio mentioned this pull request Feb 13, 2024

Derive Queryable deserialize_as attribute doesn't support tuples #3920

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve derive syntax #2626

Improve derive syntax #2626

pksunkara commented Jan 19, 2021 •

edited

Loading

weiznich left a comment

pksunkara commented Jan 20, 2021

weiznich commented May 31, 2021

pksunkara commented May 31, 2021

pksunkara commented Jun 30, 2021

weiznich commented Jun 30, 2021

pksunkara commented Jul 7, 2021 •

edited

Loading

pksunkara commented Jul 7, 2021 •

edited

Loading

weiznich left a comment

pksunkara commented Jul 13, 2021

weiznich commented Jul 13, 2021

pksunkara commented Jul 13, 2021

pksunkara commented Sep 22, 2021 •

edited

Loading

weiznich left a comment

weiznich commented Nov 23, 2021

pksunkara commented Nov 23, 2021

weiznich commented Nov 23, 2021

pksunkara commented Dec 6, 2021

weiznich commented Dec 6, 2021

pksunkara commented Dec 9, 2021

pksunkara commented Dec 9, 2021

weiznich left a comment

weiznich Dec 9, 2021

weiznich commented Dec 9, 2021

pksunkara commented Dec 10, 2021

weiznich left a comment

weiznich commented Dec 10, 2021

pksunkara commented Dec 10, 2021

Improve derive syntax #2626

Improve derive syntax #2626

Conversation

pksunkara commented Jan 19, 2021 • edited Loading

weiznich left a comment

Choose a reason for hiding this comment

pksunkara commented Jan 20, 2021

weiznich commented May 31, 2021

pksunkara commented May 31, 2021

pksunkara commented Jun 30, 2021

weiznich commented Jun 30, 2021

pksunkara commented Jul 7, 2021 • edited Loading

pksunkara commented Jul 7, 2021 • edited Loading

weiznich left a comment

Choose a reason for hiding this comment

pksunkara commented Jul 13, 2021

weiznich commented Jul 13, 2021

pksunkara commented Jul 13, 2021

pksunkara commented Sep 22, 2021 • edited Loading

weiznich left a comment

Choose a reason for hiding this comment

weiznich commented Nov 23, 2021

pksunkara commented Nov 23, 2021

weiznich commented Nov 23, 2021

pksunkara commented Dec 6, 2021

weiznich commented Dec 6, 2021

pksunkara commented Dec 9, 2021

pksunkara commented Dec 9, 2021

weiznich left a comment

Choose a reason for hiding this comment

weiznich Dec 9, 2021

Choose a reason for hiding this comment

weiznich commented Dec 9, 2021

pksunkara commented Dec 10, 2021

weiznich left a comment

Choose a reason for hiding this comment

weiznich commented Dec 10, 2021

pksunkara commented Dec 10, 2021

pksunkara commented Jan 19, 2021 •

edited

Loading

pksunkara commented Jul 7, 2021 •

edited

Loading

pksunkara commented Jul 7, 2021 •

edited

Loading

pksunkara commented Sep 22, 2021 •

edited

Loading