Add a 'lines' text writer format #415

jobarr-amzn · 2022-09-09T22:27:19Z

Description of changes:
I needed to generate newline-centric Ion data from binary and didn't have an easy way to do it, so I ended up doing something more involved with regular expressions, a small perl script, and reasoning about indentation. This is the capability that I wish I had then.

I've also added a space_between_top_values parameter to the RawTextWriterBuilder so that top-level values can be handled differently, and extracted a heap-allocated WhitespaceConfig struct as suggested in the comment around raw text writer whitespace configuration.

Please let me know wherever this can be improved, my Rust is as of yet very unoxidized.

Representatives of each format, from the documentation:

default

{foo: 1, bar: 2, baz: 3} [1, 2, 3] true "hello"

lines

{foo: 1, bar: 2, baz: 3}
[1, 2, 3]
true
"hello"

pretty

{
    foo: 1,
    bar: 2,
    baz: 3
}
[
    1,
    2,
    3
]
true
"hello"

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jobarr-amzn · 2022-09-09T22:28:56Z

I've already patched ion-cli to support --format newline as well, and will fire that PR out after this ships. Happy to take suggestions on the newline name as well. oneline? Something else?

rmarrowstone · 2022-09-09T23:07:30Z

src/text/raw_text_writer.rs

            }
        } else {
            // Otherwise, this is not the first value in this container. Emit the container's
            // delimiter (for example: in a list, write a `,`) before we write the value itself.
            self.write_value_delimiter()?;
-            write!(&mut self.output, "{}", self.space_between_values)?;


Did you consider hooking into the write_value_delimiter fn that's already switching on ContainerType or otherwise combining this with that logic?

I did look at it, but I didn't see a satisfactory way. If I had to put this in write_value_delimiter then I'd still have to switch there on whether or not to add the whitespace_config.space_between_values. It also looks to me like write_value_delimiter is focused on the syntactic delimiters, not the presentation/whitespace additions.

I'd rather leave all the "spacing" concerns in one place, I think.

This also makes me wonder whether the builder ought to validate that all the supplied whitespace values are semantically whitespace.

rmarrowstone · 2022-09-09T23:10:59Z

Per your note, I don't love the 'newline' name either. I don't have a great suggestion, but would suggest something like 'distinct' or '(per|each)(Item|datum)' something something...

jobarr-amzn · 2022-09-09T23:20:32Z

Oof, this didn't show up in cargo build locally but I need to address the new TextKind in ion_c_writer.rs#L17.

I'll have to go to back to the drawing board a bit.

codecov · 2022-09-21T21:03:24Z

Codecov Report

Merging #415 (58a19c3) into main (b53ba24) will increase coverage by 0.00%.
The diff coverage is 83.78%.

@@           Coverage Diff           @@
##             main     #415   +/-   ##
=======================================
  Coverage   83.40%   83.41%           
=======================================
  Files          83       83           
  Lines       15813    15909   +96     
=======================================
+ Hits        13189    13270   +81     
- Misses       2624     2639   +15

Impacted Files	Coverage Δ
src/text/text_writer.rs	`78.72% <0.00%> (-7.33%)`	⬇️
src/value/writer.rs	`61.53% <0.00%> (+4.39%)`	⬆️
src/text/raw_text_writer.rs	`87.50% <87.73%> (+1.60%)`	⬆️
src/lib.rs	`73.04% <0.00%> (-1.04%)`	⬇️
src/text/raw_text_reader.rs	`90.58% <0.00%> (+0.16%)`	⬆️
src/reader.rs	`82.40% <0.00%> (+0.30%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

zslayton

Nice job! Some superficial comments below.

src/text/raw_text_writer.rs

zslayton · 2022-09-23T12:50:20Z

src/text/raw_text_writer.rs

    ) -> RawTextWriterBuilder {
-        self.space_after_field_name = space_after_container_start.into();
+        self.whitespace_config.space_after_field_name = space_after_container_start;


There's some dissonance here that I probably caused but can't remember why. Should space_after_container_start and space_after_field_name have two separate setters? Or should we unify their names somehow?

I think this is just a bug? Here's the relevant snippet of the compact text format configuration:

// Single space between field names and values space_after_field_name: " ", // The first value in a container appears next to the opening delimiter space_after_container_start: "",

and here's the relevant snippet of the pretty text format configuration:

// Field names and values are separated by a single space space_after_field_name: " ", // The first value in a container appears on a line by itself space_after_container_start: "\n",

Both of them have different values for these two fields, and those different values will be used. It's only this setter which conflates them, and no tests exercise this setter.

I added some tests that exercise the builder setters.

src/text/raw_text_writer.rs

zslayton · 2022-09-23T12:53:12Z

src/text/raw_text_writer.rs

+    // Otherwise use the compact/default layout from `DEFAULT_WS_CONFIG`
+    ..DEFAULT_WS_CONFIG


zslayton · 2022-09-23T12:54:05Z

src/text/raw_text_writer.rs

-    indentation: String,
-    space_after_field_name: String,
-    space_after_container_start: String,
+    whitespace_config: Box<WhitespaceConfig>,


Nice, this drops 96 bytes of stack space down to 8! 🙌

src/text/text_writer.rs

zslayton

Excellent! 🙌

There's a clippy suggestion about using !is_empty instead of != "". Could you address that before merging?

jobarr-amzn requested a review from zslayton September 9, 2022 22:27

rmarrowstone reviewed Sep 9, 2022

View reviewed changes

Add a 'lines' text writer format

2aa1b4b

jobarr-amzn force-pushed the newline-text-format branch from dabd570 to 2aa1b4b Compare September 21, 2022 20:51

Add comments

dbf6f11

jobarr-amzn and others added 2 commits September 21, 2022 15:16

Appease rustfmt

69b15fc

Merge branch 'main' into newline-text-format

d02c216

zslayton reviewed Sep 23, 2022

View reviewed changes

jobarr-amzn added 2 commits October 2, 2022 13:10

Responding to PR comments

1594a0d

cargo fmt

01bb55a

zslayton approved these changes Oct 3, 2022

View reviewed changes

fix clippy issue

5ec60a9

jobarr-amzn changed the title ~~Add a 'newline' text writer format~~ Add a 'lines' text writer format Oct 3, 2022

Merge branch 'main' into newline-text-format

58a19c3

jobarr-amzn merged commit ec8384d into main Oct 3, 2022

jobarr-amzn deleted the newline-text-format branch October 3, 2022 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 'lines' text writer format #415

Add a 'lines' text writer format #415

jobarr-amzn commented Sep 9, 2022 •

edited

jobarr-amzn commented Sep 9, 2022

rmarrowstone Sep 9, 2022

jobarr-amzn Sep 9, 2022

rmarrowstone commented Sep 9, 2022

jobarr-amzn commented Sep 9, 2022

codecov bot commented Sep 21, 2022 •

edited

zslayton left a comment

zslayton Sep 23, 2022

jobarr-amzn Oct 1, 2022

jobarr-amzn Oct 2, 2022

zslayton Sep 23, 2022

zslayton Sep 23, 2022

zslayton left a comment

		// Otherwise use the compact/default layout from `DEFAULT_WS_CONFIG`
		..DEFAULT_WS_CONFIG

Add a 'lines' text writer format #415

Add a 'lines' text writer format #415

Conversation

jobarr-amzn commented Sep 9, 2022 • edited

jobarr-amzn commented Sep 9, 2022

rmarrowstone Sep 9, 2022

Choose a reason for hiding this comment

jobarr-amzn Sep 9, 2022

Choose a reason for hiding this comment

rmarrowstone commented Sep 9, 2022

jobarr-amzn commented Sep 9, 2022

codecov bot commented Sep 21, 2022 • edited

Codecov Report

zslayton left a comment

Choose a reason for hiding this comment

zslayton Sep 23, 2022

Choose a reason for hiding this comment

jobarr-amzn Oct 1, 2022

Choose a reason for hiding this comment

jobarr-amzn Oct 2, 2022

Choose a reason for hiding this comment

zslayton Sep 23, 2022

Choose a reason for hiding this comment

zslayton Sep 23, 2022

Choose a reason for hiding this comment

zslayton left a comment

Choose a reason for hiding this comment

jobarr-amzn commented Sep 9, 2022 •

edited

codecov bot commented Sep 21, 2022 •

edited