gnprice / toml-cli

MIT License
120 stars 24 forks source link

Update crates and add some sub command/options #13

Open liubin opened 2 years ago

liubin commented 2 years ago

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:

gnprice commented 1 year ago

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 commented 1 year ago

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

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

gnprice commented 1 year ago

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.

liubin commented 1 year ago

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 commented 1 year ago

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.

gnprice commented 1 year ago

OK, I've pushed the following commits to the main branch: 2fa3eec2d deps: Update Cargo.lock format to version 3 c36301c3a deps: Update lexical-core, fixing the build on newer Rust 8f93d2f46 deps: toml_edit 0.3 9c4fe0b3c deps: toml_edit 0.12.6 0acf4e314 deps: toml_edit 0.15, the latest 9219320d8 deps: cargo update c4b5564f3 lint: Silence a clippy::single_match warning 05abfbd6a lint: Silence another cosmetic clippy complaint a3c784c19 lint: Fix remaining clippy warnings d05e75508 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.