ron-rs / ron

Rusty Object Notation
https://docs.rs/ron
Apache License 2.0
3.22k stars 118 forks source link

Add `explicit_struct_names` Extension #522

Closed sylv256 closed 6 months ago

sylv256 commented 6 months ago

Fixes #519

sylv256 commented 6 months ago

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

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sylv256 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

juntyr commented 6 months ago

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 GRCOVEXCL 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 commented 6 months ago

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

juntyr commented 6 months ago

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

juntyr commented 6 months ago

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

juntyr commented 6 months ago

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