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

Revert #329 and add to_string/writer_advanced functions instead #334

Closed
wants to merge 3 commits into from

Conversation

malte-v
Copy link

@malte-v malte-v commented Nov 14, 2021

See #329 (comment)

Unlike #329, this approach preserves backwards compatibility. I've also removed some duplicate code along the way.

  • I've included my change in CHANGELOG.md

@malte-v malte-v changed the title Advanced serialize Revert #329 and add to_string/writer_advanced functions instead Nov 14, 2021
@torkleyy torkleyy requested a review from kvark November 17, 2021 16:42
Copy link
Collaborator

@kvark kvark left a comment

Choose a reason for hiding this comment

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

I trust your judgement on this. Personally I wasn't convinced by the argument in #329 (comment)
RON isn't self-describing, it relies on types, so forcing it to be distinctive for different types shouldn't be a goal.

@malte-v
Copy link
Author

malte-v commented Dec 2, 2021

Yeah, the argument in #329 (comment) doesn't that make much sense tbh. However, I still stand by the argument that the option to emit struct names does not belong in PrettyConfig because it makes a difference when deserializing, unlike the other options in PrettyConfig.

@kvark
Copy link
Collaborator

kvark commented Dec 2, 2021

Ok, that argument sounds more reasonable :)

@torkleyy
Copy link
Contributor

torkleyy commented Dec 3, 2021

@MomoLangenstein I think this interacts with your PR #343, right? Any thoughts on how to incorporate both these PRs?

@juntyr
Copy link
Member

juntyr commented Dec 3, 2021

@MomoLangenstein I think this interacts with your PR #343, right? Any thoughts on how to incorporate both these PRs?

Yes, it does! I think that this would be a fantastic option to add to the Options builder. If this PR gets merged first I could make that refactor (or I could just get my PR merge-ready more quickly). One interesting aspect would be what this option means for deserializing. It could have no impact (which would fit RON not being self-describing), or it could enforce that struct names are provided. I would prefer the former as I could imagine that many would like output nice and clear RON but also allowing more inference during deserialisation. And the point of #343 is kind of that you use the same options for both ser and de.

@torkleyy
Copy link
Contributor

torkleyy commented Dec 3, 2021

Sounds good to me! I'm wondering if we even need to expose any kind of *_advanced function with so many parameters, would probably be simpler with the Options API then.

@juntyr
Copy link
Member

juntyr commented Dec 3, 2021

Sounds good to me! I'm wondering if we even need to expose any kind of *_advanced function with so many parameters, would probably be simpler with the Options API then.

That's a good point. Right now I feel like there shouldn't be more convenience functions. I like that when you just want to get RON running you only need from_str and to_string. I feel like once you want more than just those very simple default cases, you can just initiate the Serializer or Deserializer directly.

@juntyr juntyr mentioned this pull request Feb 2, 2022
@juntyr juntyr marked this pull request as draft February 2, 2022 19:49
@malte-v
Copy link
Author

malte-v commented May 4, 2022

I don't plan to take this any further, closing.

@malte-v malte-v closed this May 4, 2022
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.

4 participants