add visit and visit_mut APIs #261

sunshowers · 2021-11-21T23:20:09Z

This is modeled after the syn::visit and syn::visit_mut APIs. See discussion in #192 (comment) for what prompted me to write this.

Also include some nontrivial examples for custom formatting.

One thing that immediately occurs to me is that we could easily provide a recursive formatter based on this (which would strengthen the argument for it being available unconditionally).

ordian · 2021-11-21T23:25:41Z

Should this be enabled by default and/or feature flagged? I currently have it flagged but I think there's a good argument that it should just be made available unconditionally.

I agree. We only gate something only if there is some overhead, like it adds a dependency.

sunshowers · 2021-11-22T05:05:34Z

Added a visit API, and removed the feature flagging. Looks pretty good now, let me know what you think!

An easy exercise would be to write a recursive formatter. I can add that in another commit on top of the stack as well.

sunshowers · 2021-11-22T05:25:16Z

Ahh this needs a --all-targets passed into cargo test in CI

examples/visit.rs

sunshowers · 2021-11-22T21:01:55Z

I've changed it so that the table and inline_table visit methods both call table_like, and there's now a single table_like_kv method which should make things simpler. I also made the example slightly more complex -- it can now normalize inline tables into tables within a scope as well.

Along the way I found a formatting bug for tables, and I ended up fixing that as well.

Seems like we may want to provide a bunch of standard inbox visitors in a follow-up (bundled with toml_edit or perhaps a library on the side):

turn inline tables underneath to standard tables
turn standard tables underneath to inline tables
recursive formatting

Visitors can also be used in a more delegated style, where visitor1 can have internal logic that switches to visitor2 under some sections of the tree.

src/visit.rs

epage · 2021-11-23T01:24:29Z

src/table.rs

+            Item::Table(table) => {
+                kv.key.decor = Decor::new(DEFAULT_KEY_PATH_DECOR.0, DEFAULT_KEY_PATH_DECOR.1);
+                table.decor = Decor::new(DEFAULT_TABLE_DECOR.0, DEFAULT_TABLE_DECOR.1);
+            }
+            Item::ArrayOfTables(array) => {
+                kv.key.decor = Decor::new(DEFAULT_KEY_PATH_DECOR.0, DEFAULT_KEY_PATH_DECOR.1);
+                for table in array.iter_mut() {
+                    table.decor = Decor::new(DEFAULT_TABLE_DECOR.0, DEFAULT_TABLE_DECOR.1);
+                }
+            }


Along the way I found a formatting bug for tables, and I ended up fixing that as well.

Table::fmt() wasn't properly applying the right decor to tables and
arrays of tables.

What is being considered a bug in the old version? Not reformatting the table headers? I think this is a question of whether you are considering it from the test document perspective or the in-memory data structure perspective. I was personally viewing this from the document perspective, that we are only formatting the visual table body. I feel like to consider this from the data structure perspective, we would need to recurse.

Yeah, at first it was about reformatting headers (I ran into an issue where converting an inline table into a standard table would cause the header to have a trailing space). Then I realized that we weren't formatting leading and trailing whitespace on sub tables or arrays of tables either.

The invariant I was going for was that calling fmt() on every element of the document would format the whole document. Currently that invariant is mostly upheld except for the cases fixed in this PR.

btw I realized there is a bug with Table::fmt for the description I gave above, #262

Yeah, at first it was about reformatting headers (I ran into an issue where converting an inline table into a standard table would cause the header to have a trailing space). Then I realized that we weren't formatting leading and trailing whitespace on sub tables or arrays of tables either.

When converting table types, we should probably reset the formatting to defaults since 99% of the time, the formatting will be counter to the users goal.

And/or we provide a "reset to default" formatting / clear formatting function.

The invariant I was going for was that calling fmt() on every element of the document would format the whole document. Currently that invariant is mostly upheld except for the cases fixed in this PR.

Thanks for sharing the intent! That is a worthwhile goal. I wonder if we should have a separate "format table header".

Either way, maybe we should split this out into a separate PR so we can discuss and iterate on it without being tied up with the rest of the work here?

sunshowers · 2021-11-23T18:09:27Z

When converting table types, we should probably reset the formatting to defaults since 99% of the time, the formatting will be counter to the users goal.

I think that makes sense. What do you think about recursive formatting? In particular I'm thinking of the situation where you're converting a standard table into an inline one, and there's multiline arrays within it (this is addressed in the visit_mut example). The TOML spec says:

Inline tables are intended to appear on a single line. A terminating comma (also called trailing comma) is not permitted after the last key/value pair in an inline table. No newlines are allowed between the curly braces unless they are valid within a value. Even so, it is strongly discouraged to break an inline table onto multiples lines. If you find yourself gripped with this desire, it means you should be using standard tables.

And/or we provide a "reset to default" formatting / clear formatting function.

How would this differ from the fmt() function that currently exists?

Thanks for sharing the intent! That is a worthwhile goal. I wonder if we should have a separate "format table header".

So when I started implementing this I was thinking along those lines as well. However, the issue is that the header is not part of the Table data structure, so the closest we can get to it is "format all table and array-of-tables headers that are children of this table".

Either way, maybe we should split this out into a separate PR so we can discuss and iterate on it without being tied up with the rest of the work here?

Ahh unfortunately the issue is that the included example actually depends on the formatting to work properly, so there's a dependency between the two. GH isn't great at expressing dependencies between pull requests (this is something I've personally talked to GH reps about -- also there are others trying to solve this problem). That's why I had to club it together under the same PR. However, I have tried to produce independent, separately reviewable commits.

sunshowers · 2021-11-23T18:11:26Z

Also, as far as recursive formatting goes, I think it would be easiest to build it on top of VisitMut rather than doing something bespoke.

sunshowers · 2021-11-23T18:16:38Z

Oh, re fmt() invariants, here are the two I was going for:

Calling fmt() on every element of a document should format the whole document.
fmt() on an individual element does the minimum necessary work to uphold invariant 1.

epage · 2021-11-23T20:03:14Z

When in the context of the stated goal, fmt makes sense. Outside of full document reformatting, it feels like unintended behavior to say "I format a table X and other table headers are being re-formatted". I think the only way for us to go down that route is if we make fmt recursive.

That said, I am unsure about making it recursive, about tying key-value pair formatting with header formatting. This feels very restrictive. For example, in cargo-edit, we check if key-value pairs are sorted and try to maintain that. That kind of inferring formatting I can see applying here. Someone is more likely to format all headers the same. In cargo-edit, we could check the header formatting and seek to preserve it. So like sorting not being a part of fmt, I see headers not being a part of fmt because they all operate on a different axis of user care abouts.

With that said, I see fmt, sort_keys, etc all being a subset of a future formatting API.

Either way, maybe we should split this out into a separate PR so we can discuss and iterate on it without being tied up with the rest of the work here?

Ahh unfortunately the issue is that the included example actually depends on the formatting to work properly, so there's a dependency between the two. GH isn't great at expressing dependencies between pull requests (this is something I've personally talked to GH reps about -- also there are others trying to solve this problem). That's why I had to club it together under the same PR. However, I have tried to produce independent, separately reviewable commits.

The dependency is because of the table conversion, correct? Let's focus on fixing that for unblocking this PR. If you want, I can start working on a PR for it.

sunshowers · 2021-11-23T20:28:41Z

The dependency is because of the table conversion, correct? Let's focus on fixing that for unblocking this PR. If you want, I can start working on a PR for it.

Both the array formatting issue (commit 1 which should be uncontroversial I think) and the table conversion block this PR, yeah.

sunshowers · 2021-11-23T20:29:09Z

(If you want to work on it for a bit, sounds great, yeah!)

sunshowers · 2021-11-23T20:32:32Z

So like sorting not being a part of fmt, I see headers not being a part of fmt because they all operate on a different axis of user care abouts.

This makes sense. I can see everything being part of a "normalize" API.

epage · 2021-11-23T21:00:59Z

In testing #265 against your branch, I realized that it won't help your case. You are already running fmt to workaround what that fixes. The problem is the key pointing to the converted table. Going to give this some thought.

epage · 2021-11-23T21:04:51Z

I feel like the crux of the problem is that the kv visitor should be given a mutable key so you can clear the existing decor. We don't want to expose a &mut Key because then someone can do a *key = <something> and get a visualization that is independent of the actual key.

I think we should add a KeyMutRef and have the iter_mut return that. The KeyMutRef is a proxy object that would allow the mutations that are safe.

A formatted array is all in one line, so it shouldn't have trailing commas or whitespace.

sunshowers · 2021-11-24T03:21:29Z

Thank you for adding KeyMut! I believe that's all that's necessary to make this work. I've removed the table fmt commit and updated VisitMut to take KeyMut.

This is modeled after the `syn::visit_mut` API. Also include a nontrivial example for custom formatting.

Similar to the visit_mut API, this allows walking over the document tree. I think overall this is probably less useful than `visit_mut`, but is still worth implementing for completeness.

The previous commits added some tests to the `visit` example -- ensure that they're run in CI.

sunshowers · 2021-11-24T03:26:09Z

Looks like all of the extra formatting work you did also obviated the need for a ton of fmt calls, so I've removed them from the example. Thank you :)

epage · 2021-11-30T15:45:23Z

.github/workflows/ci.yml

@@ -45,7 +45,7 @@ jobs:
      - uses: actions-rs/cargo@v1
        with:
          command: test
-          args: --all-features
+          args: --all-features --all-targets


Unless I'm missing something, it looks like --all-targets causes doc tests to not run. Ran into this with another project. when comparing cargo test (fails) to cargo test --all-targets (passes).

:(

yeah: rust-lang/cargo#6669

Looks like we are covered with our default feature run, so probably good enough here.

sunshowers force-pushed the visit branch from 0d8cf31 to 0cad8ab Compare November 22, 2021 05:00

sunshowers changed the title ~~[RFC] add a visit_mut API~~ add visit and visit_mut APIs Nov 22, 2021

sunshowers force-pushed the visit branch 2 times, most recently from 09d0654 to b57a894 Compare November 22, 2021 05:04

epage reviewed Nov 22, 2021

View reviewed changes

examples/visit.rs Outdated Show resolved Hide resolved

epage reviewed Nov 22, 2021

View reviewed changes

examples/visit.rs Outdated Show resolved Hide resolved

sunshowers force-pushed the visit branch from b57a894 to 20b1c35 Compare November 22, 2021 20:54

epage reviewed Nov 23, 2021

View reviewed changes

src/visit.rs Outdated Show resolved Hide resolved

epage reviewed Nov 23, 2021

View reviewed changes

sunshowers force-pushed the visit branch from 20b1c35 to 720d1d0 Compare November 23, 2021 18:31

fix: remove trailing commas and whitespace from formatted arrays

b5a2818

A formatted array is all in one line, so it shouldn't have trailing commas or whitespace.

sunshowers force-pushed the visit branch from 720d1d0 to afba38a Compare November 24, 2021 03:20

sunshowers added 3 commits November 23, 2021 19:25

feat: add a visit_mut API

bc43f74

This is modeled after the `syn::visit_mut` API. Also include a nontrivial example for custom formatting.

feat: add a visit API

c9702ec

Similar to the visit_mut API, this allows walking over the document tree. I think overall this is probably less useful than `visit_mut`, but is still worth implementing for completeness.

ci: run tests for all targets

d1a1db8

The previous commits added some tests to the `visit` example -- ensure that they're run in CI.

sunshowers force-pushed the visit branch from afba38a to d1a1db8 Compare November 24, 2021 03:25

epage merged commit d8b1f90 into toml-rs:master Nov 25, 2021

sunshowers deleted the visit branch November 25, 2021 18:13

epage reviewed Nov 30, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add visit and visit_mut APIs #261

add visit and visit_mut APIs #261

sunshowers commented Nov 21, 2021 •

edited

Loading

ordian commented Nov 21, 2021

sunshowers commented Nov 22, 2021

sunshowers commented Nov 22, 2021

sunshowers commented Nov 22, 2021

epage Nov 23, 2021

sunshowers Nov 23, 2021 •

edited

Loading

epage Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

epage commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

epage commented Nov 23, 2021

epage commented Nov 23, 2021

sunshowers commented Nov 24, 2021 •

edited

Loading

sunshowers commented Nov 24, 2021

epage Nov 30, 2021

ordian Nov 30, 2021

epage Nov 30, 2021

add visit and visit_mut APIs #261

add visit and visit_mut APIs #261

Conversation

sunshowers commented Nov 21, 2021 • edited Loading

ordian commented Nov 21, 2021

sunshowers commented Nov 22, 2021

sunshowers commented Nov 22, 2021

sunshowers commented Nov 22, 2021

epage Nov 23, 2021

Choose a reason for hiding this comment

sunshowers Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

epage Nov 23, 2021

Choose a reason for hiding this comment

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

epage commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

sunshowers commented Nov 23, 2021

epage commented Nov 23, 2021

epage commented Nov 23, 2021

sunshowers commented Nov 24, 2021 • edited Loading

sunshowers commented Nov 24, 2021

epage Nov 30, 2021

Choose a reason for hiding this comment

ordian Nov 30, 2021

Choose a reason for hiding this comment

epage Nov 30, 2021

Choose a reason for hiding this comment

sunshowers commented Nov 21, 2021 •

edited

Loading

sunshowers Nov 23, 2021 •

edited

Loading

sunshowers commented Nov 24, 2021 •

edited

Loading