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

Release v0.7.1 #382

Merged
merged 32 commits into from
Jun 15, 2022
Merged

Release v0.7.1 #382

merged 32 commits into from
Jun 15, 2022

Conversation

torkleyy
Copy link
Contributor

@torkleyy torkleyy commented Jun 6, 2022

Closes #365

TODO

  • Figure out what to do with struct_names
  • Changelog

@kvark @MomoLangenstein Only four months and four days later and I found some time to do the long awaited 0.7.1 release 😅

I've included all merged PRs since the v0.7.0 release, except:

Changelog

  • Add struct_names option to PrettyConfig (#329)
  • Fix newtype variant unwrapping around enum, seq and map (#331)
  • Implement unwrap_newtypes extension during serialization (#333)
  • Implement unwrap_variant_newtypes extension during serialization (#336)
  • Fix issue #338 value map roundtrip (#341)
  • Fix issue #289 enumerate_arrays comments (#344)
  • Report struct name in expected struct error (#342)
  • Add Options builder to configure the RON serde roundtrip (#343)
  • Fix issue #359 with DeserializeSeed support (#360)
  • Fix issue #370 with FromStr-equivalent float EBNF and Error::FloatUnderscore (#371)
  • Fix issue #374 extraneous .0 for small floats (#372)

struct_names issue

Pull Request #329 moved the struct_names from the Serializer to the PrettyConfig. This included a breaking change to Serializer::new, removing the third parameter. I see a couple ways to handle this:

a) for 0.7.x, include the field in both Serializer and PrettyConfig and generate struct names if either are true

Advantages:

  • easy to implement

Disadvantages:

  • possibly surprising if PrettyConfig and the Serializer::new argument are controlled by different people and one sets it to false

b) use the third parameter to override the field in PrettyConfig

Advantages:

  • easy to implement
  • won't change the behavior of any existing applications

Disadvantages:

  • might be surprising for end-users only controlling the PrettyConfig if the struct_names property gets overwritten -> could besolved by doing pretty_config.struct_names = pretty_config.struct_names || struct_names (but that means struct_names = false will be ignored)
  • non-pretty serialization included this option until now, but this makes struct_names no longer an option with compact serialization
  • or, alternatively, we could try constructing a dummy PrettyConfig which ends up emulating non-pretty serialization

--> Were we aware that moving the field will no longer allow struct names with non-pretty serialization when merging this change?

c) consider reverting the change

There are a couple things I don't like about this change:

  • struct names can no longer be emitted with compact serialization
  • pretty options can break functionality in unexpected ways if the RON is parsed by a different party using enums instead of structs for their serde structures or a different RON parser (seems fairly unlikely)

But I don't feel very strong about it. Having the field in PrettyConfig provides more comfort in most situations IMO, but the edge cases above could be a problem sometimes.

torkleyy and others added 27 commits June 6, 2022 10:02
* Fix some clippy warnings

* Fix some more clippy warnings

Co-authored-by: Rémi Lauzier <remilauzier@protonmail.com>
Fix newtype variant unwrapping inside enum, seq and map
Enable `unwrap_newtypes` extension during serialization
Added improved `ExpectedStruct` error message
Add compact arrays (ron-rs#299) and pretty inline separators
Add struct name mismatch error + better error messages
* Move struct_names from Serializer to PrettyConfig

* Update changelog
* Add code coverage CI step

* Added codecov badge to README

* Exclude examples from codecov collection
Expose `Extensions` during ser+de through `ron::Options`
…-rs#372)

Co-authored-by: KirbyER <7432155-KirbyER@users.noreply.gitlab.com>
* Increased coverage for unit tests

* Removed unused enum from empty set test
src/ser/mod.rs Outdated
note = "Serializer::new is deprecated because struct_names was moved to PrettyConfig"
)]
pub fn new(writer: W, config: Option<PrettyConfig>, struct_names: bool) -> Result<Self> {
todo!("figure out how to handle struct_names");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needs to be resolved before merging.

@juntyr
Copy link
Member

juntyr commented Jun 6, 2022

Why were the other three PRs excluded (struct_names names sense) - are they breaking changes for v0.8?

@torkleyy
Copy link
Contributor Author

torkleyy commented Jun 6, 2022

Yes exactly, they are breaking changes.

@juntyr
Copy link
Member

juntyr commented Jun 6, 2022

For v0.8 I think the struct_names would be a better fit for the Options since it might have an impact on parsing. This would likely be done by passing it into the constructor like it was before the PR, or by adding a separate post-construction modification method. So perhaps we could add it to Options already, add the post-construction method, and add a depreciation notice to the Serializer constructor that in the next version one should use Options or the post-construction modifier?

@torkleyy
Copy link
Contributor Author

torkleyy commented Jun 6, 2022

since it might have an impact on parsing

Hmm, it wouldn't right now, would it? I don't think there's a way to get the name of a struct when using deserialize_any, but this is just off the top of my head.
Despite that, I'm thinking it might still be the most consistent option.

@torkleyy torkleyy requested a review from kvark June 7, 2022 14:37
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.

Anything to resolve until CI passes and this is published?

@torkleyy
Copy link
Contributor Author

torkleyy commented Jun 13, 2022

@kvark CI is failing intentionally (nevermind, fixed now), we only need to resolve how to handle struct_names as described in the PR description. Any thoughts on it?

This is temporary change that should be reverted once
the next minor version gets released.
It enables semver compatibility with the last release,
and is necessary for the 0.7.1 release
@torkleyy
Copy link
Contributor Author

Okay, I fixed CI and implemented option a.

@torkleyy torkleyy requested a review from juntyr June 13, 2022 18:57
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2022

Codecov Report

❗ No coverage uploaded for pull request base (v0.7@7aba9c2). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             v0.7     #382   +/-   ##
=======================================
  Coverage        ?   88.87%           
=======================================
  Files           ?       47           
  Lines           ?     4982           
  Branches        ?        0           
=======================================
  Hits            ?     4428           
  Misses          ?      554           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aba9c2...b3bef7f. Read the comment docs.

@juntyr
Copy link
Member

juntyr commented Jun 13, 2022

@torkleyy Ok, that looks good to me 👍 Perhaps we should document the planned depreciation in the release notes?

@juntyr
Copy link
Member

juntyr commented Jun 15, 2022

LGTM

@torkleyy
Copy link
Contributor Author

Thanks for reviewing @MomoLangenstein

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.

6 participants