bleep icon indicating copy to clipboard operation
bleep copied to clipboard

Add `--version` command line option

Open SlowBrainDude opened this issue 1 year ago • 14 comments

I've got confused a few time which bleep build I'm currently calling.

There is no --version option at the moment so I added one.

I didn't find a way to do it "less hacky" as when using the decline lib more idiomatic together with a proper command one would always get other output before the version string. This is not good for scripting as you would need more complex logic to parse the version.

(A suggestion for a follow up: I think just using decline without any hackery around it would make things more simple and easier to maintain. Also it's sub-optimal imho to have "hidden" CLI options like bsp because of this hackery.)

I think there could be more output following the line with the version string, like for example a link to the homepage, or a license hint, but I'm not sure what would be preferred here, so it's currently just the name of the tool and its version.

In case there are suggestion to add more output I'm happy to update the PR!

SlowBrainDude avatar Sep 12 '23 13:09 SlowBrainDude

Thanks for this as well :)

I'm positive, but can it be a command instead of a flag? bleep --version => bleep version? or what do you think?

and just to comment it, the version string is also printed out in the wall of text if you run just bleep

oyvindberg avatar Sep 15 '23 23:09 oyvindberg

running bleep fmt should fix CI I guess

oyvindberg avatar Sep 15 '23 23:09 oyvindberg

Sorry for the formatting, was an oversight (likely also in the other PRs…).

I've tried now a few CLI tools with sub-commands, like rustup, git, scala-cli. Seems they use both forms (a switch and sub-command) for versions.

I've seen of course that bleep outputs the version string somewhere in the middle of the help output. But that would be almost impossible to parse form a script in a stable way.

I'm not entirely convinced that the order bleep outputs things conforms to the usual expectations. But as I don't know any standard covering that (and I didn't dig for one) I'm not touching this: As a user you want indeed the help output most when you ask for help, or input something unsupported. Putting the name and version first like in a lot of other tools is a distraction, imho. So maybe bleep does already the most sensible thing.

SlowBrainDude avatar Sep 16 '23 12:09 SlowBrainDude

Giving up on this for now.

It does not work like it should. But fixing this would require likely some more involved refactoring of how the CLI argument parsing works.

The --version / version thingy doesn't show up in the help output when bleep is run in an uninitialized directory… :disappointed:

Also now the code got way to hacky for my liking. Throwing around asInstanceOf just to get some help output (not even at the right place…) is too much.

I'm not sure this PR is the right place to discuss a refactoring of the argument parsing. I guess a dedicated ticket would be better. (I don't think I should start doing anything without getting your opinions on that first, as this would be likely a little bit more than just a few lines changed…)

Also I would like to have some possibility to actually test automatically what I'm doing. I've forgot about some things while trying manually. That's not good. The other PR has some basic CLI invocation tests. Would like to expend on this also for this feature here.

Thanks for your patience!

SlowBrainDude avatar Sep 16 '23 14:09 SlowBrainDude

Alright, so just to explain. I know the whole main file is a bit complicated, but there are some reasons of course.

It's really 4 different programs in one. zsh completions, bash completions, bsp and bleep. These all have very little overlap, and I don't want the three former to appear in the documentation for the latter.

Furthermore the bleep part of the main is divided in two, depending on whether a valid build is found or not. A different set of commands is available in those two cases. The reason two code paths is needed is that scripts names are injected from a valid build into the cli commands, so you can say bleep scriptname instead of bleep run-script scriptname (the latter does not exist)

The commands for the case without a build are referenced in noBuildOpts, while the others are in hasBuildOpts. If you want to add a bleep version command, you just refer it from those two methods. You'll typically extract the common things into another def which is again referenced in both, just like importCmd for instance.

oyvindberg avatar Sep 16 '23 21:09 oyvindberg

Also I would like to have some possibility to actually test automatically what I'm doing. I've forgot about some things while trying manually. That's not good. The other PR has some basic CLI invocation tests. Would like to expend on this also for this feature here.

I think this is a good exercise. I have not written enough tests for bleep so far. The approach you have come up with here is fine. The only thing I would change is to not mutably change stdin and stdout for the entire jvm, but pass as parameters instead. we could also do that in a followup.

oyvindberg avatar Sep 16 '23 21:09 oyvindberg

Alright, so just to explain. I know the whole main file is a bit complicated, but there are some reasons of course.

I've got that. The code is actually quite well written so the intend was clear (after looking at it for some time).

But I'm not sure I agree with the logic behind it. If you have in fact a few different tools than these should be released as such. But I would say, you don't have a bunch of independent tools. You have one tool with a broad set of features.

That's where sub-commands come in. The whole idea of sub-commands is to have "multiple tools in one". Things that are related on principle but actually quite independent features.

Most CLI tools don't have sub-commands as they try to "do one thing and do it well". But there are "feature monsters" like for example git. For such tools it makes very much sense to be structured around sub-commands.

I think one could just promote one of the sub-commands to a default or something like that. (Not entirely sure how this should work. The std. Unix way of doing something like that would be to call the binary through different names; but this would be problematic with the single binary deployment; you would need some installer script to setup links; but hmm, you have an installer for shell completion built-in :thinking:)

The basic structure of the commands seems fine. And I think the intend to separate docs a little bit is also a good one.

The main point is: I think it could be implemented more stream-lined.

The main "offenders" I've experienced were the early outputs on stdOut. Also the early splitted code paths for a build directory and some empty directory seems artificial. Also there is this branching to run an external versions of bleep…

I don't have a good idea currently how to refactor this. Didn't think to hard about it. That's the reason I've said I don't want to touch it currently. I see there's reasoning behind, and even I have qualms I don't know how to do it better.

I guess the idea would be to split the actual logic of main form the command line framework. At the moment the framework comes first, and your logic is kind of "hacked in". The framework seems not flexible in some regards. So maybe doing it the opposite way and putting your logic first and using the command line parsing framework in a second step? But just thinking out loud, like said don't have a good plan. Just the feeling that something is off if it's kind of "hard" to add a --version switch.

Regardless:

The current implementation is frankly anyway trash. It's again not testable. System.exit() is pure evil when it comes to tests. That's why I had to create a "fake main" in the other PR…

For this case here one would need some "early return", because the unrelated outputs otherwise. So exceptions or so, as the Scala native features for early returns are not portable between Scala 2 and 3…

Also I thought once more about whether version should be a switch, a sub-command, or both:

I think the switch is actually the right thing. It switches the main application into a mode where it just outputs the version and exist. So "switch" is here the right abstraction.

The sub-command seems redundant… Git has it, and Scala-CLI has it. But rustup does not. I was mislead as it just outputs the version and help when you use an illegal sub-command, but does not tell you that it was wrong explicitly (bad UX, rustup, bad UX…). I don't know of more examples as sub-commands are actually quite rare.

But one could have still a useful "version" command as a sub-command of the "build" sub-command, I guess. This one would not output the version of bleep currently installed on the system but the one configured for the build. But such thing is more part of some "info" command of the "build" sub-command, I guess… So not sure it would be useful on it's own. Or maybe "build version" would output also other version of the stack like Bloop version, and / or BSP version?

But before continuing on that I really wish it could be implement without "hacks". The empty Opts thingies and the hacked code path (especially with that exit!) look just too terrible.

Also I think you should consider to embrace the sub-commands for what they're good…

Sorry for the long post!

I was thinking about that for some time and wanted to include most results so I don't forget myself. :smile:

SlowBrainDude avatar Sep 17 '23 19:09 SlowBrainDude

Look, the structure is fine. It has scaled well to support a whole variety of challenges,and will scale much longer. It has ~one System.exit, and you proved you can easily extract that out as well.

You can refactor it all you want, but there is no way you'll get a much fancier way of expressing the logic which backs for instance this example:

$ bleep -d ../other-dir compile foo<TAB>

Here's what bleep will do:

  • early-parse -d in CommonOpts to intercept loading of the build, since that indicates it lives in another folder
  • parse that build. let's say it's successful
  • notice that that build indicates another version of bleep.
  • download and run that
  • branch into the completion part of the code.
  • discover that the current command is compile, and we're parsing vararg arguments for it
  • consult the build to come up with projectnames starting with foo
  • print completions to stdout

bleep version versus bleep --version

I know --version is implemented as a flag in many other tools, but within decline it does not make sense that way. it doesn't modify a command - it is a command. You'll find no way of implementing a flag without a command.

oyvindberg avatar Sep 17 '23 19:09 oyvindberg

Thanks for the reply!

Look, the structure is fine. It has scaled well to support a whole variety of challenges,and will scale much longer.

That's why I think the logic is just fine, as said before.

I'm just not sure about the implementation. This does not mean much, as I'm new to this code. Just my gut feeling.

Your example is interesting indeed. Thanks for that! :+1:

It has ~one System.exit, and you proved you can easily extract that out as well.

But this PR would add another one… I didn't find a way around unrelated output otherwise.

This one would be more tricky to refactor out than the other. Like said, I guess I need to throw exceptions… That's really ugly, in addition to the other ugly things I've done here.

You can refactor it all you want, […]

I'm not sure I want that… :smile:

Still I'm thinking how to get rid of the ugly stuff… But you say decline doesn't like that. Hmm.

I know --version is implemented as a flag in many other tools […]

And that's how it should be. There are expectations. When you're building a CLI app follow the common approach. Otherwise your app will stick out as alien body.

I guess most people would expect any CLI app to have a --help and a --version switch.

I didn't look though standards yet, though.

but within decline it does not make sense that way

Than what decline does doesn't make sense (maybe).

I was actually thinking a few times whether this should be actually addressed in decline. They provide a default --help switch, maybe they should also provide a default --version switch?

What do you think, does it belong there?

it doesn't modify a command - it is a command

Well, a switch does something on the command it's set. The --version switch changes the behavior of the main command, and this is bleep.

In an app that has sub-commands it could be also one of these. But imho it wouldn't make much sense if it was redundant, but it would be redundant in case the sub-command just does the same as the switch and doesn't provide additional switches, options, or even further sub-commands (like in git, which actually isn't the best example of a clean CLI interface :grinning:).

You'll find no way of implementing a flag without a command.

Than this is a shortcoming of decline.

It claims to support idiomatic command line interfaces. So it should do according. :smile:

But besides that, maybe there is some sense in a version sub-command (likely as "sub-sub-command"). In case it does something else than the switch.

SlowBrainDude avatar Sep 17 '23 21:09 SlowBrainDude

I didn't find a way around unrelated output otherwise.

Can you accept the unrelated output for now? It's mostly benign I think.

My plan for "clean" output is something like what kubectl for instance does, where you normally unstructured text output, but you can pass --out json to get no-nonsense json output. I think this would be super for building scripts around bleep.

Than this is a shortcoming of decline.

can we just roll with the flow here and do it the way decline is modelled? super easy to implement, and even if you may slightly disagree with it it makes perfect sense in my head

oyvindberg avatar Sep 17 '23 21:09 oyvindberg

Can you accept the unrelated output for now? It's mostly benign I think.

I was thinking also that this may be just temporary.

The output could be restructured later on.

I for example think that outputting how long the "bootstrap" of bleep took is irrelevant for normal operation.

Also maybe the output of the "bleep version switch" could be made somehow else to not appear when asking for the version?

(Also the "flashlights", I mean the colored Unicode thingies in front of each line, which don't follow the console theme, are questionable. But that's likely a different discussion.)

can we just roll with the flow here and do it the way decline is modelled? super easy to implement, and even if you may slightly disagree with it it makes perfect sense in my head

I'm really not sure about that.

Like said, a version sub-command makes some sense, but not much imho. (Except you come up with something different it could inform about?). But the version switch is more or less obligatory. (Please don't make me read crazy Unix specs. :smiley:)

So I'm not against having the sub-command really (git does it, so it must be good… :smile:). But I wanted actually the switch. One does not know whether an app has sub-commands. But you expect at least some basic switches…

(Maybe this would also make things like dev-ops scripts simpler as you could abstract about a bunch of CLI tools when collecting version information. Parsing ${COMMAND} --version is likely used somewhere. Needing extra care for some tools which don't follow common sense is always annoying. Looking at you java… :unamused:)

My plan for "clean" output is something like what kubectl for instance does, where you normally unstructured text output, but you can pass --out json to get no-nonsense json output. I think this would be super for building scripts around bleep.

This is of course cool to have as an additional feature.

But I would really expect to have things like --help and --version on a command.

If you say this is a decline thingy, well, than I'll go there demanding a std. --version switch. :grin:

If you want to merge "something" ASAP than feel free to demand the required changes and I'll carry on with it.

But like said, for me the goal of this exercise was to have a proper --version switch as I would find it helpful when you have a few builds of bleep for testing purposes, and a switch is what my muscle memory outputs to ask for that info.

SlowBrainDude avatar Sep 17 '23 22:09 SlowBrainDude

Yes, exactly.

There is already functionality in place to add/remove output.

  • try for instance bleep config log-timing-enable and then run a slow command like compile. you'll see timing numbers on the left of the screen
  • likewise, you can add --debug to all commands to see much more output.

so something like that i suppose, maybe a "power user mode" or whatever. haven't really thought much in that direction

oyvindberg avatar Sep 17 '23 22:09 oyvindberg

I've done a little research in the meantime.

My gut feeling about having the expectation that every CLI tool should support --help and --version is now backed by some more formal recommendations:

https://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html

https://www.gnu.org/prep/standards/html_node/_002d_002dversion.html#g_t_002d_002dversion

Of course the GNU standards aren't authoritative (like most other pieces of paper… :smile:) but let's say there is quite a strong user expectation about CLI tools working like that (as most of them are build around the GNU standards).

I'm not sure my previous comment here was read in full, as I submitted it too early in the middle of writing it and I think GitHub doesn't notify about edits…

To make it short: I could live with a sub-command-only solution for now (and in case this is wanted I update the PR accordingly) but I would still pursue to somehow have a proper --version switch in the future.

SlowBrainDude avatar Sep 22 '23 23:09 SlowBrainDude

I could live with a sub-command-only solution for now (and in case this is wanted I update the PR accordingly) but I would still pursue to somehow have a proper --version switch in the future.

Let's go! a sub-command (which shows up in --help and autocomplete), and optionally you can match at the top-level if args == List("--version") and then do the same, this will agree with peoples muscle memory. both should in theory be very easy

oyvindberg avatar Sep 23 '23 09:09 oyvindberg