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

Add field-specific functionality to Into #291

Merged
merged 50 commits into from
Oct 19, 2023

Conversation

MegaBluejay
Copy link
Contributor

@MegaBluejay MegaBluejay commented Aug 11, 2023

Resolves #287

Synopsis

Allow using the same attribute arguments on fields as on the whole struct, generate impls for each

Solution

Extract the common arguments of struct and field attributes into a seperate parser

See details on proposed semantics of using both together in the issue

Checklist

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

tests/into.rs Outdated
Comment on lines 1056 to 1066
impl From<Tuple> for (i32, f64, f64) {
fn from(value: Tuple) -> Self {
(value.0, value.1, value.2)
}
}

impl From<Tuple> for (i32, f64) {
fn from(value: Tuple) -> Self {
(value.0, value.2)
}
}
Copy link
Owner

@JelteF JelteF Aug 13, 2023

Choose a reason for hiding this comment

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

Why are these impls here? If they are there on purpose they could use a code comment.

Copy link
Contributor Author

@MegaBluejay MegaBluejay Aug 14, 2023

Choose a reason for hiding this comment

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

The idea was to test that no extra impls get generated, similarly to this IntoIterator test

For instance, no impl should be generated using all the fields if there's at least one field attribute, and if there are types specified in an attribute the field's type shouldn't get one either.

@MegaBluejay MegaBluejay marked this pull request as ready for review August 14, 2023 09:17
@tyranron tyranron added this to the 1.0.0 milestone Aug 14, 2023
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.

@MegaBluejay please, read the #287 (comment), where the design was finalized, and consider it.

A case currently not supported is skipping fields for which other arguments are also present, e.g.

#[derive(Into)]
#[into]
struct Foo {
    #[into(skip, String)]
    a: String,
    b: i32
    c: f64,
}

to generate Foo -> (i32, f64) and Foo -> String impls.

This isn't mentioned in the issue, and I'm not sure how useful it would be

This should be definitely supported as separate attributes:

#[derive(Into)]
#[into]
struct Foo {
    #[into(String)]
    #[into(skip)]
    a: String,
    b: i32
    c: f64,
}

@MegaBluejay MegaBluejay requested a review from tyranron August 14, 2023 12:02
@MegaBluejay MegaBluejay changed the title Restore field-specific functionality to Into Add field-specific functionality to Into Aug 14, 2023
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.

@MegaBluejay please, consider to reuse #303 changes in this PR.

@MegaBluejay MegaBluejay marked this pull request as ready for review October 9, 2023 09:57
@MegaBluejay MegaBluejay requested a review from tyranron October 9, 2023 10:06
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.

@MegaBluejay thanks! Overall: good job! 🍻

I've simplified your solution a bit, as introducing generic Alt and Pair structural types encoding just for removing Option merging boilerplate feels a little bit too much. Adding ParseMultiple::merge_opt_attrs() should do the job as good, while keeping less amount of code overall.

@JelteF since this PR introduces new API, please take a look.

Also, after merging this PR, would you be so kind to release 1.0.0-beta.4 version?

} else {
Pair {
left: L::default(),
right: input.parse()?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@MegaBluejay if parsing left has failed, we have the broken state on input stream already, so parsing right would likely fail as well. Whenever we try to parse something, we should use .fork()ing to omit changing the initial state of the input stream.

@tyranron tyranron requested a review from JelteF October 12, 2023 11:15
@tyranron
Copy link
Collaborator

ping @JelteF

@JelteF
Copy link
Owner

JelteF commented Oct 19, 2023

The new API looks good to me

@JelteF JelteF merged commit 50ae43b into JelteF:master Oct 19, 2023
16 checks passed
@JelteF
Copy link
Owner

JelteF commented Oct 19, 2023

I released v1.0.0-beta.6 which includes this change. (cargo release was causing some problems so the beta version was bumped a few times more than intended)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

derive(Into) lost the ability to convert into particular fields
3 participants