Adds config option for writing struct field names as text/sid #816
Conversation
| fn config(&self) -> ValueWriterConfig { | ||
| v1_0::Binary::default_value_writer_config() | ||
| } |
There was a problem hiding this comment.
🗺️ The StructWriter impls need to be able to configure a value writer for each field. Only the application-level value writer types (ListWriter, StructWriter, etc) actually hold a ValueWriterConfig (at least for the time being), so the raw writer types just return the default ValueWriterConfig for their encoding.
| @@ -59,7 +59,9 @@ impl<'value, 'top> BinaryValueWriter_1_1<'value, 'top> { | |||
| } | |||
|
|
|||
| pub fn with_inline_symbol_text(mut self) -> Self { | |||
| self.value_writer_config = self.value_writer_config.with_delimited_containers(); | |||
There was a problem hiding this comment.
🗺️ This was a copy/paste error that made it through in an earlier PR.
| @@ -741,7 +739,7 @@ macro_rules! annotate_and_delegate_1_1 { | |||
| let value_writer = $crate::lazy::encoder::binary::v1_1::value_writer::BinaryValueWriter_1_1::new( | |||
| self.allocator, | |||
| self.buffer, | |||
| self.config(), | |||
There was a problem hiding this comment.
🗺️ Previously, fn config(&self) -> ValueWriterConfig was a required method on ValueWriter and MakeValueWriter, but I realized it wasn't used in many places.
This PR moves the requirement to the StructWriter trait, which only has ~6 impls.
| Self { | ||
| name, | ||
| struct_writer, | ||
| value_writer_config, |
There was a problem hiding this comment.
🗺️ Constructing a FieldWriter now takes a ValueWriterConfig. Application-level constructs use their own, lower-level constructs just use their encoding's default for now.
| pub fn new() -> Self { | ||
| ValueWriterConfig::default() | ||
| /// Constructs a `ValueWriterConfig` that writes all symbol tokens as inline text. | ||
| pub const fn text() -> Self { |
There was a problem hiding this comment.
🗺️ Constructors that can be used as a jumping-off point in a const context.
| symbol_creation_policy, | ||
| supports_text_tokens, |
There was a problem hiding this comment.
🗺️ These config options have been replaced by ValueWriterConfig.
| self.raw_value_writer = self | ||
| .raw_value_writer |
There was a problem hiding this comment.
🗺️ These used to delegate to their inner raw writer, but I realized that was just storing application-level config at a lower level. Now the application-level types store their own config.
| } | ||
| } | ||
|
|
||
| pub fn with_field_name_encoding(mut self, field_name_encoding: FieldNameEncoding) -> Self { |
There was a problem hiding this comment.
🗺️ Notice that this lives on the struct writer, so you don't do
writer.value_writer().with_field_name_encoding(E)you do
writer.struct_writer().with_field_name_encoding(E)which is a bit more obvious.
| self.encoding, | ||
| self.value_writer_config, | ||
| self.raw_struct_writer.make_value_writer(), | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| impl<'value, V: ValueWriter> FieldEncoder for ApplicationStructWriter<'value, V> { | ||
| fn encode_field_name(&mut self, name: impl AsRawSymbolRef) -> IonResult<()> { |
There was a problem hiding this comment.
🗺️ The changes in this method are what cause the field name encoding config setting to be respected.
| @@ -150,6 +154,14 @@ impl Encoding for BinaryEncoding_1_0 { | |||
| fn default_write_config() -> WriteConfig<Self> { | |||
| WriteConfig::<Self>::new() | |||
| } | |||
|
|
|||
| fn default_value_writer_config() -> ValueWriterConfig { | |||
There was a problem hiding this comment.
🗺️ Each encoding can now provide its default ValueWriterConfig, which is what a Writer starts with.
src/lazy/encoder/writer.rs
Outdated
| // Symbols that are already in the symbol table are written as SIDs | ||
| (RawSymbolRef::Text("name"), &[0xF9, 0x6E, 0x61, 0x6D, 0x65]), // FlexSym 4, SID $4, |
There was a problem hiding this comment.
This looks like a copy-paste mistake from one of the other tests. Is it?
There was a problem hiding this comment.
Yeah. The code is right but the comments are wrong. I'll fix this before merging, thanks.
Plumbs a
ValueWriterConfiginstance through the application layer value writer types.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.