Skip to content

Add explicit_struct_names Extension#522

Merged
juntyr merged 27 commits into
ron-rs:masterfrom
sylv256:master
Jan 7, 2024
Merged

Add explicit_struct_names Extension#522
juntyr merged 27 commits into
ron-rs:masterfrom
sylv256:master

Conversation

@sylv256

@sylv256 sylv256 commented Jan 3, 2024

Copy link
Copy Markdown
Contributor
  • I've included my change in CHANGELOG.md

Fixes #519

Comment thread src/de/mod.rs Outdated
@sylv256 sylv256 marked this pull request as ready for review January 4, 2024 03:12

@sylv256 sylv256 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

added some questions and comments

Comment thread src/extensions.rs Outdated
Comment thread src/parse.rs Outdated
Comment thread tests/522_explicit_struct_names.rs Outdated
@sylv256

sylv256 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

FYI Before you merge, I'm adding some documentation Finished ✨

@sylv256 sylv256 requested a review from juntyr January 4, 2024 03:49
Comment thread src/error.rs Outdated
@codecov-commenter

codecov-commenter commented Jan 4, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e27d1d5) 100.00% compared to head (4bf6ef7) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #522   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           82        83    +1     
  Lines        14410     14443   +33     
=========================================
+ Hits         14410     14443   +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@juntyr juntyr 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.

Overall, the changes all look good - thank you for also adding documentation!

One question that we should still resolve is how this extension relates to the PrettyConfig::struct_names option. I think it would make sense that enabling the extension during serialisation forces all struct names to be emitted (even when we're not in pretty serialising mode). Do we then still need the PrettyConfig::struct_names option? I think if we can still emit struct names with and without (as two distinct options) emitting the explicit #[enable(explicit_struct_names)] marker, we can probably also axe the config option.

@sylv256

sylv256 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

I think it would be more intuitive to keep the serialization configuration options separate from the deserialization extension. If PrettyConfig is only used for serialization, then it would make sense to put all of the serialization options in one place. If we go that route, we should add a "see also" link to the documentation for PrettyConfig::struct_names and vice versa.

@sylv256

sylv256 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

If PrettyConfig is only used for serialization, then it would make sense to put all of the serialization options in one place. If we go that route, we should add a "see also" link to the documentation for PrettyConfig::struct_names and vice versa.

It turns out PrettyConfig does use Extensions, so I think we should just drop PrettyConfig::struct_names.

@juntyr

juntyr commented Jan 4, 2024

Copy link
Copy Markdown
Member

I think it would be more intuitive to keep the serialization configuration options separate from the deserialization extension. If PrettyConfig is only used for serialization, then it would make sense to put all of the serialization options in one place. If we go that route, we should add a "see also" link to the documentation for PrettyConfig::struct_names and vice versa.

So far, all extensions have served a dual purpose of specifying a variation of the RON format and enabling it during both serialisation and deserialisation, so that you can produce a RON document with the same options (that’s how the Options struct originated) that you will apply during parsing. Therefore, I think it would be good for consistency to keep that dual functionality.

So far, explicit struct names have just been a prettifying option, but this PR obviously changes that. If we keep the PrettyConfig option as well, the extension would be used in non-pretty mode, but we’d need to decide how to merge the extension and the pretty config - or-ing them makes most sense.

Either way, whether we keep the PrettyConfig option or not, I agree with you that we should add a short explanation to each item.

Comment thread src/extensions.rs
@juntyr

juntyr commented Jan 4, 2024

Copy link
Copy Markdown
Member

Could you also bring back the coverage to 100%? There is a test in the error file where you can just add another case. Then, in your own test, you could either try to use asserts or manually disable coverage for the unreachable panics (search for GRCOV_EXCL_ in other tests for examples).

All in all, thank you so much for working on this addition! Once these last minor nits are addressed, it’ll be ready to be merged.

@sylv256

sylv256 commented Jan 4, 2024

Copy link
Copy Markdown
Contributor Author

I believe I've fixed the formatting and coverage now.

Comment thread src/error.rs Outdated
Comment thread src/error.rs Outdated
Comment thread src/ser/mod.rs Outdated
Comment thread tests/522_explicit_struct_names.rs
@juntyr

juntyr commented Jan 4, 2024

Copy link
Copy Markdown
Member

Ok, just a couple more minor nits - thank you again for implementing the extension!

Comment thread src/ser/mod.rs Outdated
Comment thread src/ser/mod.rs
@juntyr

juntyr commented Jan 5, 2024

Copy link
Copy Markdown
Member

I believe I've fixed the formatting and coverage now.

Yes, thank you! It seems one unrelated #[derive(Debug)] line has regressed in https://app.codecov.io/gh/ron-rs/ron/pull/522/blob/tests/non_string_tag.rs though

Comment thread src/extensions.rs Outdated
Co-authored-by: Juniper Tyree <50025784+juntyr@users.noreply.github.com>

@juntyr juntyr 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.

LGTM!

@juntyr juntyr merged commit d509208 into ron-rs:master Jan 7, 2024
@juntyr

juntyr commented Jan 7, 2024

Copy link
Copy Markdown
Member

@SylvKT Thank you so much again for implementing this extension!

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.

Option to Expect Specific Struct Name Upon Deserialization

3 participants