Skip to content

Adds unwrap_variant_newtypes extension#319

Merged
torkleyy merged 2 commits into
ron-rs:masterfrom
juntyr:master
Oct 20, 2021
Merged

Adds unwrap_variant_newtypes extension#319
torkleyy merged 2 commits into
ron-rs:masterfrom
juntyr:master

Conversation

@juntyr

@juntyr juntyr commented Oct 13, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

This PR fixes #250 by adding a new unwrap_variant_newtypes extension. When enabled, newtype variants will skip the outer parentheses of the wrapped type:

#[derive(Deserialize)]
pub enum Foo {
	A(A),
	B,
}

#[derive(Deserialize)]
pub struct A {
	pub b: u8,
}

In this example, Foo can now be parsed from A(b: 4).

Notes

  • By adding a new feature, no breaking change occurs.
  • By enabling the unwrap_variant_newtypes, neither A(A(b: 4)) nor A((b: 4)) will parse, as unwrapping newtype variants is always performed.
  • This extension can mimic some of the functionality of unwrap_newtypes if the newtype variant wraps around a newtype struct. Both extensions are compatible, but in this case unwrap_newtypes is not needed as the newtype variant can also perform the unwrapping.

@kvark kvark requested a review from torkleyy October 15, 2021 19:12
torkleyy
torkleyy reviewed Oct 18, 2021
View reviewed changes

@torkleyy torkleyy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not quite sure yet if we should use the implemented behavious (forbidding parantheses unless the struct name is given) - I'm not sure if this might be confusing.

Regarding breaking changes, on a first glance it doesn't look like a breaking change but I need to read through it again. I wouldn't worry about bumping version though, because RON barely ever gets exposed so upgrading really isn't an issue.

@juntyr

juntyr commented Oct 18, 2021

Copy link
Copy Markdown
Member Author

My reasoning was to make sure that the parsing is unambiguous. If parentheses were still accepted but optional, we wouldn’t know if they are part of the wrapped struct or its inner content … though maybe this could be solved by eagerly eating them but remembering that we were eager. If we then check for parentheses but can’t find any and this flag is set, we could infer that the parentheses were part of the inner type (and then apply that knowledge when parsing the outer parenthesis). However, this logic might be a bit complicated …

@torkleyy

torkleyy commented Oct 18, 2021

Copy link
Copy Markdown
Member

Thanks for the explanation! @MomoLangenstein
That certainly makes sense. What do you think about not allowing to write out the newtype with the extension enabled?

@juntyr

juntyr commented Oct 18, 2021

Copy link
Copy Markdown
Member Author

@torkleyy So forbidding A(A(b: 4)) and A((b: 4)), only allowing A(b: 4)? That would be an easy change and make the rules quite clear - it would always unwrap the newtype. I’d be very ok with that as well

@toyboot4e toyboot4e mentioned this pull request Oct 19, 2021
Closed
@torkleyy

torkleyy commented Oct 19, 2021

Copy link
Copy Markdown
Member

Yes, exactly @MomoLangenstein, that's what I was thinking. If you can do that, I'd be willing to merge this - except @kvark has any concerns?

@kvark

kvark commented Oct 19, 2021

Copy link
Copy Markdown
Collaborator

That seems fine, and useful. I appreciate the strictness proposed. Let's not forget to adjust the PR description accordingly.

@juntyr

juntyr commented Oct 19, 2021

Copy link
Copy Markdown
Member Author

Yes, exactly @MomoLangenstein, that's what I was thinking. If you can do that, I'd be willing to merge this - except @kvark has any concerns?

Brilliant, I'll change up the PR then

@juntyr

juntyr commented Oct 19, 2021

Copy link
Copy Markdown
Member Author

Yes, exactly @MomoLangenstein, that's what I was thinking. If you can do that, I'd be willing to merge this - except @kvark has any concerns?

Brilliant, I'll change up the PR then

@torkleyy I have implemented the changes and updated & extended the tests. I've also added a quick extension description inside the docs folder - I hope this has the right format?

torkleyy
torkleyy approved these changes Oct 20, 2021
View reviewed changes

@torkleyy torkleyy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lovely PR, with tests & docs, thank you @MomoLangenstein :shipit:

bors r+

@bors

bors Bot commented Oct 20, 2021

Copy link
Copy Markdown
Contributor

Configuration problem:
bors.toml: not found

@torkleyy torkleyy merged commit c76f946 into ron-rs:master Oct 20, 2021
@juntyr

juntyr commented Oct 20, 2021

Copy link
Copy Markdown
Member Author

Thanks @torkleyy for your help and guidance!

@juntyr juntyr mentioned this pull request Oct 29, 2021
Merged
1 task
@juntyr juntyr mentioned this pull request Nov 16, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@torkleyy torkleyy torkleyy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Avoiding duplicate parens around enum variant members (related to newtype-wrapping)

3 participants

@juntyr @torkleyy @kvark