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

Redesign Into derive macro #248

Merged
merged 59 commits into from
Jul 15, 2023
Merged

Redesign Into derive macro #248

merged 59 commits into from
Jul 15, 2023

Conversation

ilslv
Copy link
Contributor

@ilslv ilslv commented Mar 16, 2023

Synopsis

This PR is a part of replacing all attributes having syn::Meta syntax to custom parsing.

Solution

Replace #[into(types(i32, "&str"))] with #[from(i32, &str)] and add support for deriving multi-field structs and enum variants with #[from((<tuple>), (<tuple>), ...)].

Checklist

  • Documentation is updated (if required)
  • Tests are added/updated (if required)
  • CHANGELOG entry is added (if required)

ilslv and others added 30 commits February 9, 2023 15:21
# Conflicts:
#	CHANGELOG.md
#	README.md
#	impl/Cargo.toml
#	impl/doc/debug.md
#	impl/doc/display.md
#	impl/src/fmt/debug.rs
#	impl/src/fmt/display.rs
#	impl/src/fmt/mod.rs
#	impl/src/fmt/parsing.rs
#	impl/src/lib.rs
#	src/fmt.rs
#	tests/compile_fail/debug/both_fmt_and_skip_on_field.stderr
#	tests/compile_fail/debug/legacy_bound_syntax.stderr
#	tests/compile_fail/debug/legacy_fmt_syntax.stderr
#	tests/compile_fail/debug/unknown_attribute.stderr
#	tests/compile_fail/display/unknown_attribute.stderr
@ilslv ilslv marked this pull request as ready for review March 17, 2023 13:29
@JelteF JelteF added this to the 1.0.0 milestone Mar 18, 2023
@JelteF
Copy link
Owner

JelteF commented Mar 18, 2023

It's a bit hard to review this as it also contains all the changes from #241. Could you create a PR on your own where the base branch is the from-attribute branch? That way the diff doesn't contain so much unrelated stuff and it's easier to comment on the actual changes that are made for this one.

thing to note is that this derive doesn't actually generate an implementation
for the `Into` trait. Instead, it derives `From` for the values contained in
the struct and thus has an indirect implementation of `Into` as
[recommended by the docs][1].
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
[recommended by the docs][1].
[recommended by the Rust docs for `Into`][1].

impl/doc/into.md Outdated
Comment on lines 27 to 28
For structs that have multiple fields `.into()` extract tuple containing the
desired content for each field.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
For structs that have multiple fields `.into()` extract tuple containing the
desired content for each field.
For structs that have multiple fields `.into()` extracts a tuple containing the
desired content for each field.

impl/doc/into.md Outdated
The behaviour is a bit different when deriving for a struct with multiple
fields, since it returns a tuple. For instance when deriving for a tuple struct
with two fields like this:
To specify concrete types to derive convert into use `#[into(<types>)]`.
Copy link
Owner

@JelteF JelteF Mar 18, 2023

Choose a reason for hiding this comment

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

This sentence is a bit strange, maybe something like this:

Suggested change
To specify concrete types to derive convert into use `#[into(<types>)]`.
Use `#[into(<types>)]` to specify the concrete types that you want to convert the type into.

}
```
#[derive(Debug, Into, PartialEq)]
#[into(owned, ref(i32), ref_mut)]
Copy link
Owner

@JelteF JelteF Mar 18, 2023

Choose a reason for hiding this comment

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

So into(owned) is equivalent to into(types({current_type}))? Do we really need owned then?

Copy link
Owner

Choose a reason for hiding this comment

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

I think a better example would be to use ref(i64)

Copy link
Owner

Choose a reason for hiding this comment

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

hmm, actually what's the difference between ref(i64) and simply using &i64?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@JelteF here ref/ref_mut control Self type representation, while &i64 controls target type.

ref(i64) means impl Into<&i64> for &MyType, while &i64 means only impl Into<&i64> for MyType.

@JelteF
Copy link
Owner

JelteF commented Jul 8, 2023

@ilslv @tyranron now that #241 is merged, let's rebase this one. That should make it much easier to review.

@ilslv
Copy link
Contributor Author

ilslv commented Jul 8, 2023

@JelteF I was planning to do it next week 👍

# Conflicts:
#	CHANGELOG.md
#	Cargo.toml
#	impl/doc/from.md
#	impl/doc/into.md
#	impl/src/fmt/mod.rs
#	impl/src/from.rs
#	impl/src/lib.rs
#	impl/src/parsing.rs
#	tests/compile_fail/from/legacy_enum_attribute.stderr
#	tests/compile_fail/from/legacy_struct_attribute.stderr
#	tests/compile_fail/from/multiple_enum_attributes.stderr
#	tests/compile_fail/from/multiple_struct_attributes.stderr
#	tests/compile_fail/from/struct_tuple_no_parens.stderr
#	tests/compile_fail/from/struct_tuple_too_long.stderr
#	tests/compile_fail/from/struct_tuple_too_short.stderr
#	tests/into.rs
Copy link
Collaborator

@tyranron tyranron left a comment

Choose a reason for hiding this comment

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

@ilslv I've unified some repeated code (in utils module you've referred to from via cfg clause, but no actual use has been introduced), and corrected docs a llitle bit. Everything else seem nice enough.

@JelteF would you like to take a look, or we can merge it?

@JelteF JelteF merged commit fdf7fe1 into JelteF:master Jul 15, 2023
@MegaBluejay MegaBluejay mentioned this pull request Aug 9, 2023
3 tasks
tyranron added a commit that referenced this pull request Aug 18, 2023
## Synopsis

This PR is a part of replacing all attributes having `syn::Meta` syntax
to custom parsing, similarly to #241, #248.

Paves the way for #285, and possibly other enhancements such as an
opt-in #123.


## Solution

Implement custom attribute parsing without `utils::State`.

Co-authored-by: Kai Ren <tyranron@gmail.com>
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.

3 participants