gnprice / toml-cli

MIT License
120 stars 24 forks source link

Let `toml set` edit the actual file #7

Open Busyvar opened 2 years ago

Busyvar commented 2 years ago

Hi,

As many other tools like sed , i think it would be interesting to confirm file editing with a flag instead of using stdout. So the argument after 'set' won't be repeated in the command.

What do you think?

Regards, Busyvar

kurtbuilds commented 2 years ago

FYI I added this in my fork:

https://github.com/kurtbuilds/toml-cli

You have to git clone it, then cargo install --path ., since the fork isn't on crates.io.

Since this project appears to be unmaintained, I reached out to Greg about contributing to this repo & crates.io to keep this tool maintained.

Busyvar commented 2 years ago

Thank you @kurtbuilds , i saw you made a PR with it, https://github.com/gnprice/toml-cli/pull/8

Hope you will be soon accepted as a maintainer for this project.

gnprice commented 1 year ago

Hello! Yes, I definitely agree this is an important feature.

One question this issue thread is a good place to discuss is what exactly the interface should look like. I'd be glad to hear feedback on that from anyone who's used this toml command.

From discussion at https://github.com/gnprice/toml-cli/pull/13#discussion_r1038729485 on @liubin's PR #13, here's one thought: instead of just making this an option, how about we go further and 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 --in-place (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.

gnprice commented 1 year ago

Now, making this happen by default would be 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.

However, I'd certainly be interested in hearing feedback on that incompatible change: does anyone have the existing toml set running in scripts, in a way where they'd be concerned that a sudden change to operating in place would break something?

If so, there's also an intermediate option to take a more sequenced approach, something like:

That way we end up with the same behavior as discussed in the previous comment https://github.com/gnprice/toml-cli/issues/7#issuecomment-1336105312 , where toml set does indeed set the value in the given file, but we also have an intermediate period of warnings and/or errors to help people spot any call sites that need to be updated.

gnprice commented 1 year ago

OK, I'm going to go with some version of the "more sequenced approach" described in my previous comment https://github.com/gnprice/toml-cli/issues/7#issuecomment-1336106542. The endpoint will be that a plain toml set actually updates the file.

I plan to call the options --write (for the good behavior, the future default) and --print (for the current behavior.)

The warning/error messages that transitional versions print on a plain toml set (telling you to add an explicit --print or --write) will be loud — designed to be easy to spot in a long build log.


One thing that informs this decision is that I went and did a bit of a survey of uses of toml-cli that I can readily find using GitHub search. Two observations are relevant to this thread: