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

Do not write excess indents arount text fields when serialize with serde #820

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

Mingun
Copy link
Collaborator

@Mingun Mingun commented Oct 11, 2024

This PR alternative and more full implementation of the fix for excess indents around text fields. It implements the Python scheme of writing indents:

struct Xml {
  before: String,// = "value1"
  #[serde(rename = "$text")] // or $value
  text: String,// = "text"
  after: String,// = "value2"
}

before, $text and after

<Xml>
  <before>value1</before>text<after>value2</after>
</Xml>

$text and after

<Xml>text<after>value2</after>
</Xml>

before and $text

<Xml>
  <before>value1</before>text</Xml>

$text only

<Xml>text</Xml>

The implementation never write indent after Option fields, because Option can contain a type that is sensible to the surrounding text content, for example Option<String>. Because serde does not provide the type of None when serialize it, it is impossible to be smart here and write indent in one cases but not other, so I choosed the safe variant.

As a side effect, to_writer, to_writer_with_root and Serializer not returns bool on success. The true means that is will be safe to write text (in particular, indent) after the serialized object, it will not be interpreted as a part of content during deserialization. If false will be returned, then writing additional text content may change data what deserializer will see.

Closes #655, closes #814

@codecov-commenter
Copy link

codecov-commenter commented Oct 11, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 98.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.19%. Comparing base (39b5905) to head (da88af2).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/se/content.rs 97.43% 1 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #820      +/-   ##
==========================================
+ Coverage   60.08%   60.19%   +0.10%     
==========================================
  Files          41       41              
  Lines       15975    15999      +24     
==========================================
+ Hits         9599     9630      +31     
+ Misses       6376     6369       -7     
Flag Coverage Δ
unittests 60.19% <98.75%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@Mingun
Copy link
Collaborator Author

Mingun commented Oct 12, 2024

Updated: even better solution. Now serialization returns the classification result (instead of just boolean) and you can decide what to do with it.


// Note that sequences of primitives serialized without delimiters!
value!(seq: vec![1, 2, 3] => "1\n 2\n 3");
value!(seq_empty: Vec::<usize>::new());
value!(seq: vec![1, 2, 3] => "123");
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is vec![1, 2, 3] distinguished from vec![123]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, in any way. This is what happens now, if you do not use indents.

Copy link
Collaborator Author

@Mingun Mingun Oct 15, 2024

Choose a reason for hiding this comment

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

In #823 I changed the return value in that case to error

3");
value!(tuple_struct: Tuple("first", 42) => "first\n 42");
value!(tuple_struct: Tuple("first", 42) => "first42");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm definitely missing something here.

How would Tuple("first42", 0) be distinguished from Tuple("first", 420)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The same answer as above. This is restriction of current implementation. I have a TODO to forbid serialization consequent text fields:

//TODO: add settings to disallow consequent serialization of primitives

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there an issue written up for that already, and if not can you write one and reference it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, there is no issue yet

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In #823 I changed the return value in that case to error

Mingun and others added 5 commits October 13, 2024 22:26
That means, never indent "$text" fields and do not indent "$value" fields that
produces string without surrounding tags

failures (208):
    se::content::tests::with_indent::enum_with_text_field::char_amp
    se::content::tests::with_indent::enum_with_text_field::char_apos
    se::content::tests::with_indent::enum_with_text_field::char_gt
    se::content::tests::with_indent::enum_with_text_field::char_lt
    se::content::tests::with_indent::enum_with_text_field::char_non_escaped
    se::content::tests::with_indent::enum_with_text_field::char_quot
    se::content::tests::with_indent::enum_with_text_field::char_space
    se::content::tests::with_indent::enum_with_text_field::enum_unit
    se::content::tests::with_indent::enum_with_text_field::enum_unit_escaped
    se::content::tests::with_indent::enum_with_text_field::f32_
    se::content::tests::with_indent::enum_with_text_field::f64_
    se::content::tests::with_indent::enum_with_text_field::false_
    se::content::tests::with_indent::enum_with_text_field::i128_
    se::content::tests::with_indent::enum_with_text_field::i16_
    se::content::tests::with_indent::enum_with_text_field::i32_
    se::content::tests::with_indent::enum_with_text_field::i64_
    se::content::tests::with_indent::enum_with_text_field::i8_
    se::content::tests::with_indent::enum_with_text_field::isize_
    se::content::tests::with_indent::enum_with_text_field::newtype
    se::content::tests::with_indent::enum_with_text_field::option_none
    se::content::tests::with_indent::enum_with_text_field::option_some
    se::content::tests::with_indent::enum_with_text_field::option_some_empty_str
    se::content::tests::with_indent::enum_with_text_field::seq
    se::content::tests::with_indent::enum_with_text_field::seq_empty
    se::content::tests::with_indent::enum_with_text_field::str_escaped
    se::content::tests::with_indent::enum_with_text_field::str_non_escaped
    se::content::tests::with_indent::enum_with_text_field::true_
    se::content::tests::with_indent::enum_with_text_field::tuple
    se::content::tests::with_indent::enum_with_text_field::tuple_struct
    se::content::tests::with_indent::enum_with_text_field::u128_
    se::content::tests::with_indent::enum_with_text_field::u16_
    se::content::tests::with_indent::enum_with_text_field::u32_
    se::content::tests::with_indent::enum_with_text_field::u64_
    se::content::tests::with_indent::enum_with_text_field::u8_
    se::content::tests::with_indent::enum_with_text_field::unit
    se::content::tests::with_indent::enum_with_text_field::unit_struct
    se::content::tests::with_indent::enum_with_text_field::unit_struct_escaped
    se::content::tests::with_indent::enum_with_text_field::usize_
    se::content::tests::with_indent::enum_with_value_field::char_amp
    se::content::tests::with_indent::enum_with_value_field::char_apos
    se::content::tests::with_indent::enum_with_value_field::char_gt
    se::content::tests::with_indent::enum_with_value_field::char_lt
    se::content::tests::with_indent::enum_with_value_field::char_non_escaped
    se::content::tests::with_indent::enum_with_value_field::char_quot
    se::content::tests::with_indent::enum_with_value_field::char_space
    se::content::tests::with_indent::enum_with_value_field::f32_
    se::content::tests::with_indent::enum_with_value_field::f64_
    se::content::tests::with_indent::enum_with_value_field::false_
    se::content::tests::with_indent::enum_with_value_field::i128_
    se::content::tests::with_indent::enum_with_value_field::i16_
    se::content::tests::with_indent::enum_with_value_field::i32_
    se::content::tests::with_indent::enum_with_value_field::i64_
    se::content::tests::with_indent::enum_with_value_field::i8_
    se::content::tests::with_indent::enum_with_value_field::isize_
    se::content::tests::with_indent::enum_with_value_field::newtype
    se::content::tests::with_indent::enum_with_value_field::option_none
    se::content::tests::with_indent::enum_with_value_field::option_some
    se::content::tests::with_indent::enum_with_value_field::option_some_empty_str
    se::content::tests::with_indent::enum_with_value_field::seq
    se::content::tests::with_indent::enum_with_value_field::seq_empty
    se::content::tests::with_indent::enum_with_value_field::str_escaped
    se::content::tests::with_indent::enum_with_value_field::str_non_escaped
    se::content::tests::with_indent::enum_with_value_field::true_
    se::content::tests::with_indent::enum_with_value_field::tuple
    se::content::tests::with_indent::enum_with_value_field::tuple_struct
    se::content::tests::with_indent::enum_with_value_field::u128_
    se::content::tests::with_indent::enum_with_value_field::u16_
    se::content::tests::with_indent::enum_with_value_field::u32_
    se::content::tests::with_indent::enum_with_value_field::u64_
    se::content::tests::with_indent::enum_with_value_field::u8_
    se::content::tests::with_indent::enum_with_value_field::usize_
    se::content::tests::with_indent::seq
    se::content::tests::with_indent::text_field::enum_struct
    se::content::tests::with_indent::tuple
    se::content::tests::with_indent::tuple_struct
    se::element::tests::with_indent::text_field::map::char_amp
    se::element::tests::with_indent::text_field::map::char_apos
    se::element::tests::with_indent::text_field::map::char_gt
    se::element::tests::with_indent::text_field::map::char_lt
    se::element::tests::with_indent::text_field::map::char_non_escaped
    se::element::tests::with_indent::text_field::map::char_quot
    se::element::tests::with_indent::text_field::map::char_space
    se::element::tests::with_indent::text_field::map::enum_unit
    se::element::tests::with_indent::text_field::map::enum_unit_escaped
    se::element::tests::with_indent::text_field::map::f32_
    se::element::tests::with_indent::text_field::map::f64_
    se::element::tests::with_indent::text_field::map::false_
    se::element::tests::with_indent::text_field::map::i128_
    se::element::tests::with_indent::text_field::map::i16_
    se::element::tests::with_indent::text_field::map::i32_
    se::element::tests::with_indent::text_field::map::i64_
    se::element::tests::with_indent::text_field::map::i8_
    se::element::tests::with_indent::text_field::map::isize_
    se::element::tests::with_indent::text_field::map::newtype
    se::element::tests::with_indent::text_field::map::option_some
    se::element::tests::with_indent::text_field::map::seq
    se::element::tests::with_indent::text_field::map::str_escaped
    se::element::tests::with_indent::text_field::map::str_non_escaped
    se::element::tests::with_indent::text_field::map::true_
    se::element::tests::with_indent::text_field::map::tuple
    se::element::tests::with_indent::text_field::map::tuple_struct
    se::element::tests::with_indent::text_field::map::u128_
    se::element::tests::with_indent::text_field::map::u16_
    se::element::tests::with_indent::text_field::map::u32_
    se::element::tests::with_indent::text_field::map::u64_
    se::element::tests::with_indent::text_field::map::u8_
    se::element::tests::with_indent::text_field::map::usize_
    se::element::tests::with_indent::text_field::struct_::char_amp
    se::element::tests::with_indent::text_field::struct_::char_apos
    se::element::tests::with_indent::text_field::struct_::char_gt
    se::element::tests::with_indent::text_field::struct_::char_lt
    se::element::tests::with_indent::text_field::struct_::char_non_escaped
    se::element::tests::with_indent::text_field::struct_::char_quot
    se::element::tests::with_indent::text_field::struct_::char_space
    se::element::tests::with_indent::text_field::struct_::enum_unit
    se::element::tests::with_indent::text_field::struct_::enum_unit_escaped
    se::element::tests::with_indent::text_field::struct_::f32_
    se::element::tests::with_indent::text_field::struct_::f64_
    se::element::tests::with_indent::text_field::struct_::false_
    se::element::tests::with_indent::text_field::struct_::i128_
    se::element::tests::with_indent::text_field::struct_::i16_
    se::element::tests::with_indent::text_field::struct_::i32_
    se::element::tests::with_indent::text_field::struct_::i64_
    se::element::tests::with_indent::text_field::struct_::i8_
    se::element::tests::with_indent::text_field::struct_::isize_
    se::element::tests::with_indent::text_field::struct_::newtype
    se::element::tests::with_indent::text_field::struct_::option_none
    se::element::tests::with_indent::text_field::struct_::option_some
    se::element::tests::with_indent::text_field::struct_::option_some_empty_str
    se::element::tests::with_indent::text_field::struct_::seq
    se::element::tests::with_indent::text_field::struct_::seq_empty
    se::element::tests::with_indent::text_field::struct_::str_escaped
    se::element::tests::with_indent::text_field::struct_::str_non_escaped
    se::element::tests::with_indent::text_field::struct_::true_
    se::element::tests::with_indent::text_field::struct_::tuple
    se::element::tests::with_indent::text_field::struct_::tuple_struct
    se::element::tests::with_indent::text_field::struct_::u128_
    se::element::tests::with_indent::text_field::struct_::u16_
    se::element::tests::with_indent::text_field::struct_::u32_
    se::element::tests::with_indent::text_field::struct_::u64_
    se::element::tests::with_indent::text_field::struct_::u8_
    se::element::tests::with_indent::text_field::struct_::unit
    se::element::tests::with_indent::text_field::struct_::unit_struct
    se::element::tests::with_indent::text_field::struct_::unit_struct_escaped
    se::element::tests::with_indent::text_field::struct_::usize_
    se::element::tests::with_indent::value_field::map::char_amp
    se::element::tests::with_indent::value_field::map::char_apos
    se::element::tests::with_indent::value_field::map::char_gt
    se::element::tests::with_indent::value_field::map::char_lt
    se::element::tests::with_indent::value_field::map::char_non_escaped
    se::element::tests::with_indent::value_field::map::char_quot
    se::element::tests::with_indent::value_field::map::char_space
    se::element::tests::with_indent::value_field::map::f32_
    se::element::tests::with_indent::value_field::map::f64_
    se::element::tests::with_indent::value_field::map::false_
    se::element::tests::with_indent::value_field::map::i128_
    se::element::tests::with_indent::value_field::map::i16_
    se::element::tests::with_indent::value_field::map::i32_
    se::element::tests::with_indent::value_field::map::i64_
    se::element::tests::with_indent::value_field::map::i8_
    se::element::tests::with_indent::value_field::map::isize_
    se::element::tests::with_indent::value_field::map::newtype
    se::element::tests::with_indent::value_field::map::option_some
    se::element::tests::with_indent::value_field::map::seq
    se::element::tests::with_indent::value_field::map::str_escaped
    se::element::tests::with_indent::value_field::map::str_non_escaped
    se::element::tests::with_indent::value_field::map::true_
    se::element::tests::with_indent::value_field::map::tuple
    se::element::tests::with_indent::value_field::map::tuple_struct
    se::element::tests::with_indent::value_field::map::u128_
    se::element::tests::with_indent::value_field::map::u16_
    se::element::tests::with_indent::value_field::map::u32_
    se::element::tests::with_indent::value_field::map::u64_
    se::element::tests::with_indent::value_field::map::u8_
    se::element::tests::with_indent::value_field::map::usize_
    se::element::tests::with_indent::value_field::struct_::char_amp
    se::element::tests::with_indent::value_field::struct_::char_apos
    se::element::tests::with_indent::value_field::struct_::char_gt
    se::element::tests::with_indent::value_field::struct_::char_lt
    se::element::tests::with_indent::value_field::struct_::char_non_escaped
    se::element::tests::with_indent::value_field::struct_::char_quot
    se::element::tests::with_indent::value_field::struct_::char_space
    se::element::tests::with_indent::value_field::struct_::f32_
    se::element::tests::with_indent::value_field::struct_::f64_
    se::element::tests::with_indent::value_field::struct_::false_
    se::element::tests::with_indent::value_field::struct_::i128_
    se::element::tests::with_indent::value_field::struct_::i16_
    se::element::tests::with_indent::value_field::struct_::i32_
    se::element::tests::with_indent::value_field::struct_::i64_
    se::element::tests::with_indent::value_field::struct_::i8_
    se::element::tests::with_indent::value_field::struct_::isize_
    se::element::tests::with_indent::value_field::struct_::newtype
    se::element::tests::with_indent::value_field::struct_::option_none
    se::element::tests::with_indent::value_field::struct_::option_some
    se::element::tests::with_indent::value_field::struct_::option_some_empty_str
    se::element::tests::with_indent::value_field::struct_::seq
    se::element::tests::with_indent::value_field::struct_::seq_empty
    se::element::tests::with_indent::value_field::struct_::str_escaped
    se::element::tests::with_indent::value_field::struct_::str_non_escaped
    se::element::tests::with_indent::value_field::struct_::true_
    se::element::tests::with_indent::value_field::struct_::tuple
    se::element::tests::with_indent::value_field::struct_::tuple_struct
    se::element::tests::with_indent::value_field::struct_::u128_
    se::element::tests::with_indent::value_field::struct_::u16_
    se::element::tests::with_indent::value_field::struct_::u32_
    se::element::tests::with_indent::value_field::struct_::u64_
    se::element::tests::with_indent::value_field::struct_::u8_
    se::element::tests::with_indent::value_field::struct_::usize_
@Mingun Mingun merged commit aad62ba into tafia:master Oct 18, 2024
7 checks passed
@Mingun Mingun deleted the fix-indents branch October 18, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug serde Issues related to mapping from Rust types to XML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keep specific element to single line while using indent with serde serializer
4 participants