ktfmt icon indicating copy to clipboard operation
ktfmt copied to clipboard

Document @file CLI arguments

Open grodin opened this issue 2 years ago • 10 comments

Parsing args from a file if the first argument is @filename was introduced in this commit but doesn't seem to be documented anywhere apart from a cryptic hint in cli help. I only found it because I got curious so went digging in the source, but it's actually a useful feature which deserves documenting!

grodin avatar Mar 06 '23 16:03 grodin

@grodin, that's a common practice for CLIs and often CLI tools offer that right out of the box, but sure we could document that.

Do you want to submit a PR for it?

hick209 avatar May 03 '24 13:05 hick209

Can do. Should have some time next week.

grodin avatar May 03 '24 13:05 grodin

Starting work on this, I'd forgotten that the CLI is pretty barebones (e.g. lacks --help flag, only shows generic usage when failing to parse command line). Would you be interested in me improving the UX of the CLI a bit (adding --help mainly)?

Or should I just document the @argfile feature minimally, in the CLI and on the website?

Happy to do either, but might be overstepping the mark a bit with the first if you're wanting to keep the CLI ultra-minimal.

grodin avatar May 07 '24 14:05 grodin

I'd be happy to have it improved! 😃

Just maybe make it into different PRs, if possible

hick209 avatar May 07 '24 14:05 hick209

Yep, no problem

grodin avatar May 07 '24 15:05 grodin

How far can I go in rebuilding the CLI?

One option is to go the whole hog and use clikt but obviously that's a new dependency and possibly bloating a pretty small Jar.

The current CLI behaviour is encoded in tests. Should I try to keep those tests passing or can I replace them?

grodin avatar May 07 '24 16:05 grodin

Although I love clikt and use it in other projects, we intentionally didn't use it (or other cli libs) to keep the library slim.

Feel free to go as deep as you want on refactoring. Once you have something, even if not really, share it so we can start the discussion in case we want to change somethings up

hick209 avatar May 07 '24 16:05 hick209

Oh, just try not to change the CLI public API, unless it's really necessary. We're going with a 1.0 release, so that could warrant an API change.

hick209 avatar May 07 '24 18:05 hick209

Oh, just try not to change the CLI public API, unless it's really necessary. We're going with a 1.0 release, so that could warrant an API change.

Some more clarity about this would be useful. The current implementation has a fair bit of unexpected behaviour: I reported #465 separately, but the other thing is that the style flags aren't mutually exclusive, e.g.

$ ktfmt --dropbox-style --set-exit-if-changed --kotlinlang-style --google-style SomeFile.kt

is valid. The last style option on the command line wins, so --google-style in this case.

I find this fairly surprising, but I'm not certain how other CLI tools handle it. I think Clikt has the same behaviour by default, but it's not completely clear from their docs.

grodin avatar May 13 '24 15:05 grodin

Oh, just try not to change the CLI public API, unless it's really necessary. We're going with a 1.0 release, so that could warrant an API change.

Some more clarity about this would be useful.

Basically I meant that we should try not to change the options/params of the program. You brought up a very good example here, with the formatting styles. Assuming that one is changed to the following:

$ ktfmt --style=dropbox

This is something I would consider a change in the CLI API.

Honestly this looks like a good change, IMHO and we could put that in the 1.0.0 release.

Regarding how to handle multiple params, from my experience, it seems common for CLIs to just take the last params specified. The Kotlin compiler, for example, does that, only issuing a warning stating that multiple values were defined and it will take the last one. In other words, I'm fine with keeping things as they are, but if you want to improve things, you could do the same approach as the Kotlin compiler, of adding a warning message when overriding options are passed in as parameters.

hick209 avatar May 13 '24 16:05 hick209

Are you still wanting the --style= change before 1.0? It seems like it's a nice little thing to have but definitely not a blocker.

grodin avatar Jun 13 '24 13:06 grodin

You can put that up as a separated PR

hick209 avatar Jun 13 '24 13:06 hick209

To be clear, this is a change that breaks the CLI API, so it will be part of 1.0.0.

Once this PR here lands, I'll make a v0.51 release and then could immediately merge the changes for --style= since that would only get shipped on the next release, which I plan to be 1.0.0

hick209 avatar Jun 13 '24 14:06 hick209