Update crates and add some sub command/options #13

liubin · 2022-11-25T06:26:34Z

Hi, @gnprice thank you for this tool.

I want to use this tool in a shell script to edit TOML files, to run this tool, I added some commits:

update old crates and compile using the latest Rust
add a check subcommand to check if a key exists(getting a non-exists key will panic)
add --overwrite option to set to write the result to the original file.
support set i64/bool values
add a GitHub action to create release tarball and container image when pushing a new tag

gnprice · 2022-12-02T06:02:34Z

Lovely, thank you @liubin! This is great to see. These look like a number of quite useful features.

It may take me a bit of time to read through these changes, but I'll aim to do so in the next few days.

Do you think you might be up for adding a few test cases for the new functionality, too? I recognize that most of the existing functionality isn't tested either, and I'm not going to block merging this on holding out for tests. But if you do have a bit of time to try adding some tests, that would be a much-appreciated contribution as well.

liubin · 2022-12-02T12:17:50Z

@gnprice I added the 9th commit, this one adds:

some basic unit tests
a Makefile with some basic targets to help development
a GitHub workflow to run tests in new pull requests to ensure the code is working. Here is an example of it.

For integration in the test/ directory, it may take some time to think about how to do it.

gnprice · 2022-12-02T18:09:22Z

Excellent, thanks! These look like quite helpful tests.

For integration in the test/ directory, it may take some time to think about how to do it.

Cool. I think for a lot of the tests one wants to write for this, the ideal form for the test is actually an integration test that operates at the CLI level, so that would be especially helpful.

For example, once we have a good pattern for writing CLI integration tests, I'd want to take the unit test that says

        let result = set_value(toml_file.clone(), "x.z", "false", opts);

and replace it with a CLI integration test that says something like

    let result = run_toml(vec!["set", filename, "x.z", "false"]);

so that it corresponds closely to writing the command line toml set "$filename" x.z false. That way (a) the test directly tests that the interface presented to users works as specified, and (b) the test doesn't depend on internal interfaces like set_value, and so leaves us free to refactor the internal interfaces all we like without having to update tests.

But I won't block on that for merging -- I'll be happy to merge a version with unit tests like these, and leave converting them to CLI-level tests as a further improvement to make in the future.

gnprice

OK! I've now read through all of this except the build/CI/release changes, and the new tests. Very happy to have these changes.

For the housekeeping changes at the start of the branch — rustfmt, clippy, upgrading dependencies and adapting to their API changes — my preference is to keep things more separated out step-by-step in the Git history, and there are a couple of spots where I'd prefer to tell clippy to be quiet rather than take its suggestion. So I'll just go ahead and push a series of commits that takes care of those in my preferred way, and this can rebase atop that.

Comments below on the interesting substantive parts, mainly about choices on what the CLI interface should look like. Definitely interested in your feedback on the interface, based on your experience writing a script using this command.

I'm also interested in the build/CI/release changes and the tests, and will look at those later. Posting this part as a review of its own just to keep things organized.

gnprice · 2022-12-03T05:05:57Z

src/main.rs

-    /// Edit the file to set some data (currently, just print modified version)
+    /// Edit the file to set some data


Cool! I think this is an especially important feature to add.

gnprice · 2022-12-03T05:11:34Z

src/main.rs

+            let now: DateTime<Utc> = Utc::now();
+            let ext = now.format("%Y%m%d-%H%M%S-%f");
+            let backup_file = format!("{}.{}", path.display(), ext);


Hmm, this format for the backup-file name feels pretty ad-hoc to me. Here's what it gets on an example run:
foo.toml.20221203-045740-563448063
It's expressing a date and time, but it doesn't align with any standard way of writing a date and time.

How about writing it in ISO 8601 format? So the example above would instead be:
foo.toml.2022-12-03T04:57:40,563448063

gnprice · 2022-12-03T05:13:18Z

src/main.rs

+        let mut output = OpenOptions::new().write(true).truncate(true).open(path)?;
+        write!(output, "{}", doc)?;


nit: could this be made simpler by using fs::write?

Or is there perhaps a subtle behavior difference that I'm missing?

gnprice · 2022-12-03T05:31:49Z

README.md

+Check if a key exists
+
+USAGE:
+    toml check <path> <query>


Interesting, can you say more about the motivation for this subcommand?

I think the way I'd expect to handle this use case in a CLI is by using the "get" command, and just redirecting the output to /dev/null if I don't actually want it.

Looking at git config — which I find to be a good source of inspiration for this command's interface, as mentioned in the README — it looks like that's exactly what it does. There's no specialized "check" action in git config --help, but instead there's this:

--get Get the value for a given key (optionally filtered by a regex matching the value). Returns error code 1 if the key was not found and the last value if multiple key values were found.

I think that'd work well for the toml command, too.

Looks like our current behavior in toml get if there's no data at the given path is to panic, eep:

$ target/debug/toml get Cargo.toml a.b ; echo $? thread 'main' panicked at 'attempted to leave type `linked_hash_map::Node<alloc::string::String, table::TableKeyValue>` uninitialized, which is invalid', /home/greg/.cargo/registry/src/github.com-1ecc6299db9ec823/linked-hash-map-0.5.2/src/lib.rs:174:52 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 101

Or after upgrading dependencies, the panic is a bit less noisy but still a panic:

$ target/debug/toml get Cargo.toml a.b ; echo $? thread 'main' panicked at 'index not found', src/main.rs:188:32 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace 101

It's panicking in the walk_tpath function.

So I think basically what we want here is to take your check_exists function, which is a non-panicky version of walk_tpath, and put that logic in walk_tpath so that toml get can give a clean error result (just like git config does) instead of panicking.

gnprice · 2022-12-03T05:54:38Z

src/main.rs

+    /// Overwrite the TOML file (default: print to stdout)
+    #[structopt(long)]
+    overwrite: bool,


How about going further: instead of just making this a option, how about we make it the default behavior of toml set?

I feel like for most use cases, you're always going to want to write the edited file back. So it'd be cleaner if you could say simply toml set foo.toml …, rather than saying toml set --overwrite (or whatever we might call this option) all the time.

That would also make it align better with the name "set" than it currently does.

We can always include an option to go back to the other behavior, with a name like --dry-run or --print.

(Yes, this is an incompatible change. But as the README says:

The command's status is experimental. The current interface does not yet serve its purposes as well as it could, and incompatible changes are anticipated.

Making toml set update the actual file on disk is exactly the sort of change I had in mind with that warning.)

gnprice · 2022-12-03T06:00:36Z

src/main.rs

+fn detect_value(value_str: &str) -> Item {
+    if let Ok(i) = value_str.parse::<i64>() {
+        value(i)
+    } else if let Ok(b) = value_str.parse::<bool>() {
+        value(b)
+    } else {
+        value(value_str)


Hmm, this feels rather too magical to me for the interface of this command. In particular, this means that even when you're just trying to write string data, you'll sometimes have it come out as an int or a bool instead, just depending on the data -- depending on exactly what string it was you were trying to store.

Or for another angle on the same thing: this approach would mean that it'd be impossible to store some strings, like "1" or "false".

For example, it'd be impossible to correctly set package.edition in a Cargo.toml file:

$ target/debug/toml set --overwrite Cargo.toml package.edition 2018 $ git diff -U0 diff --git a/Cargo.toml b/Cargo.toml index 2f232cf98..ac60e2325 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10 +10 @@ license = "MIT" -edition = "2018" +edition = 2018 $ cargo check error: failed to parse manifest at `/home/greg/w/toml-cli/Cargo.toml` Caused by: data did not match any variant of untagged enum MaybeWorkspace for key `package.edition`

Instead, what would you think about having toml set take a flag to tell it how it should parse the value? Probably "string" would still be the default -- I feel like that's probably the most common.

So e.g.

toml set foo.toml x asdf # a string toml set foo.toml x --string asdf # the same thing, explicitly toml set foo.toml x 2018 # still a string: `x = "2018"` toml set foo.toml x --int 2018 # an int: `x = 2018` toml set foo.toml x --int asdf # error toml set foo.toml x --bool false toml set foo.toml x --bool asdf # error

liubin · 2022-12-03T06:42:55Z

Hi @gnprice thanks for your review, now there are many conflicts in this PR, and this PR contains many commits, it's difficult to fix individual ones, so I'd like to freeze this PR and split it into some separated PRs.

Here I created some issues to track your comments:

gnprice · 2022-12-03T06:44:30Z

Sure, splitting this PR up sounds great.

I did just push back to your PR branch a rebased version, though, with conflicts resolved. So take a look at that and see if it's helpful in splitting things out.

It will check if the key exists. Signed-off-by: bin liu <liubin0329@gmail.com>

Signed-off-by: bin liu <liubin0329@gmail.com>

now support set bool/i64. Signed-off-by: bin liu <liubin0329@gmail.com>

Signed-off-by: bin liu <liubin0329@gmail.com>

add two opitons for set sub command: - overwrite: save the result to the TOML file. - backup: create a backup file before overwrite. Both of them are false by default. Signed-off-by: bin liu <liubin0329@gmail.com>

Signed-off-by: bin liu <liubin0329@gmail.com>

gnprice · 2022-12-03T07:16:39Z

OK, I've pushed the following commits to the main branch:
2fa3eec deps: Update Cargo.lock format to version 3
c36301c deps: Update lexical-core, fixing the build on newer Rust
8f93d2f deps: toml_edit 0.3
9c4fe0b deps: toml_edit 0.12.6
0acf4e3 deps: toml_edit 0.15, the latest
9219320 deps: cargo update
c4b5564 lint: Silence a clippy::single_match warning
05abfbd lint: Silence another cosmetic clippy complaint
a3c784c lint: Fix remaining clippy warnings
d05e755 fmt: Add some rustfmt::skip markings, and autoformat all

Now cargo fmt and cargo clippy are both clean.

I've also pushed a rebased version of this PR back to the PR branch.

liubin force-pushed the dev branch from 7a0f618 to 679a961 Compare December 2, 2022 12:08

gnprice reviewed Dec 3, 2022

View reviewed changes

liubin force-pushed the dev branch from 679a961 to 0868c62 Compare December 3, 2022 06:37

gnprice force-pushed the dev branch from 0868c62 to 5920517 Compare December 3, 2022 06:43

This was referenced Dec 3, 2022

Get on non-existing key will panic #14

Closed

Edit in place default #15

Closed

liubin mentioned this pull request Dec 3, 2022

Set int and bool values as well as strings #16

Open

liubin added 7 commits December 2, 2022 23:11

add check sub command to check if a key exists

95c73dd

It will check if the key exists. Signed-off-by: bin liu <liubin0329@gmail.com>

add realse action to release tarball for downloading

652f864

Signed-off-by: bin liu <liubin0329@gmail.com>

add Dockerfile to release container image

d695203

Signed-off-by: bin liu <liubin0329@gmail.com>

support set different data type

783e1a2

now support set bool/i64. Signed-off-by: bin liu <liubin0329@gmail.com>

fix type: flase -> false

a99be31

Signed-off-by: bin liu <liubin0329@gmail.com>

support edit file in place

63aa4c4

add two opitons for set sub command: - overwrite: save the result to the TOML file. - backup: create a backup file before overwrite. Both of them are false by default. Signed-off-by: bin liu <liubin0329@gmail.com>

add some unit tests and a workflow to run tests in pull requests.

be95a30

Signed-off-by: bin liu <liubin0329@gmail.com>

gnprice force-pushed the dev branch from 5920517 to be95a30 Compare December 3, 2022 07:14

gnprice mentioned this pull request Dec 3, 2022

Let toml set edit the actual file #7

Open

liubin mentioned this pull request Dec 3, 2022

Add some CLI tests and workflow #18

Merged

gnprice mentioned this pull request Dec 8, 2022

Automate publishing release binaries #23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update crates and add some sub command/options #13

Update crates and add some sub command/options #13

liubin commented Nov 25, 2022

gnprice commented Dec 2, 2022

liubin commented Dec 2, 2022

gnprice commented Dec 2, 2022 •

edited

Loading

gnprice left a comment

gnprice Dec 3, 2022

gnprice Dec 3, 2022

gnprice Dec 3, 2022

gnprice Dec 3, 2022

gnprice Dec 3, 2022

gnprice Dec 3, 2022

gnprice Dec 3, 2022

liubin commented Dec 3, 2022 •

edited

Loading

gnprice commented Dec 3, 2022

gnprice commented Dec 3, 2022 •

edited

Loading

		/// Edit the file to set some data (currently, just print modified version)
		/// Edit the file to set some data

		let mut output = OpenOptions::new().write(true).truncate(true).open(path)?;
		write!(output, "{}", doc)?;

Update crates and add some sub command/options #13

Are you sure you want to change the base?

Update crates and add some sub command/options #13

Conversation

liubin commented Nov 25, 2022

gnprice commented Dec 2, 2022

liubin commented Dec 2, 2022

gnprice commented Dec 2, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

gnprice Dec 3, 2022

Choose a reason for hiding this comment

liubin commented Dec 3, 2022 • edited Loading

gnprice commented Dec 3, 2022

gnprice commented Dec 3, 2022 • edited Loading

gnprice commented Dec 2, 2022 •

edited

Loading

liubin commented Dec 3, 2022 •

edited

Loading

gnprice commented Dec 3, 2022 •

edited

Loading