Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for reading command-line options from file(s) (#191) #317

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Kalin-Rudnicki
Copy link
Contributor

/claim #191

Copy link

algora-pbc bot commented Jun 8, 2024

💵 To receive payouts, sign up on Algora, link your Github account and connect with Stripe/Alipay.

@Kalin-Rudnicki
Copy link
Contributor Author

Let me know if the code implementation is acceptable, and if so, I will add docs for it.

@Kalin-Rudnicki Kalin-Rudnicki force-pushed the current/feature/args-from-file branch from e7f5c96 to 042fe3b Compare June 8, 2024 18:24
@Kalin-Rudnicki
Copy link
Contributor Author

@pablf ccing you here because wasnt able to properly @ you in discord

@pablf
Copy link
Member

pablf commented Jun 9, 2024

@Kalin-Rudnicki Hi, it looks good! Just in case, the docs refer to the docs displayed by the CLI, not the library documentation. I like the flexibility of allowing to add your custom FileArgs. If you could add some comment about this function either as a comment, maybe in runWithFileArgs, or in the docs of the library, that would be great!

"test-app",
"v0",
HelpDoc.Span.empty,
Command("cmd", options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test other CliApps with more complex commands? One Command using all the combinators should be enough. And if you could also test when using also Args and only Args with no Options, that would be great.

@@ -0,0 +1,5 @@
package zio.cli

trait FileArgsPlatformSpecific {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be private[cli].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean

private[cli] trait FileArgsPlatformSpecific {

or

trait FileArgsPlatformSpecific private[cli] {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did the first one, looked at other *PlatformSpecifc

args: List[String],
fromFiles: List[FileArgs.ArgsFromFile],
conf: CliConfig
): IO[ValidationError, CommandDirective[A]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to have fromFiles as the third parameter with a default value. Probably it's sign of a bad use, but there might be someone calling directly this method and would be a breaking change. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems reasonable

@@ -306,12 +369,15 @@ object Options extends OptionsPlatformSpecific {
def validate[A](
options: Options[A],
args: List[String],
fromFiles: List[FileArgs.ArgsFromFile],
conf: CliConfig
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing with fromFiles as last parameter.

import zio._
import java.nio.file.Path

trait FileArgs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change Args for something referring to Options because this represents Options from files and there are both Options and Args in zio-cli.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call

import java.nio.file.{Files, Path}

object LiveFileArgsSpec extends ZIOSpecDefault {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be put also in the native folder, right?

Copy link
Contributor Author

@Kalin-Rudnicki Kalin-Rudnicki Jun 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add this, yes.
I am really annoyed... Intellij creates multiple cross-platform directories:

  • js-jvm
  • js-native
  • jvm-native
    But sbt doesnt seem to correctly add these dependencies as I would expect.
    I tried originally adding this spec in jvm-native, but it didnt seem to realize anything in shared existed :/

@Kalin-Rudnicki
Copy link
Contributor Author

@pablf can you please advise where you would recommend adding notes about FileOptions to the cli help docs?

@Kalin-Rudnicki
Copy link
Contributor Author

Kalin-Rudnicki commented Jun 10, 2024

@pablf After trying to add tests for more complex commands, I realized that I think its sub-par to do command.names.headOption in order to decide what .command file to look for.

Because you could have something like:

command1 | command2

and then you would be looking for an indeterminate one of .command1 and .command2, irrelevant of which path it tries to parse.

Im not super familiar with this library, but IMO, it seems to me that Subcommands[A, B](parent: Command[A], is too loose on the typing.
Ive written my own CLI library, and I went a much simpler approach, where my version of a Command was either exclusively a Leaf or a Branch. Aka: going down a sub-command does no parsing, other than popping a command off the front of the args list.
TBH, I think this is a better approach, but would be a pretty big change to the lib...
It seems confusing to me to figure out what it would mean to parse SubCommand where parent has non-empty Args. I'm assuming it would behave something like this?
base-cmd base-cmd--arg-1 base-cmd--arg-1 sub-cmd-1 sub-cmd-2 sub-cmd-2--arg-1

As I mentioned, this would be a pretty large change, and I don't really want to go through that, even if you agreed it was a better model.
That being said, do you think it would be fair to make it so that only Single can have .subCommands? (requires a little more than that, in order to handle .map(???).subCommands(???), a sort of SingleLike)
This would make it much easier to determine what the root command was.

Also, it seems a bit strange to do something like

val cmd1: Command[?] = ??? // complex command with orElse and subCommands
val cmd2: Command[?] = ??? // complex command with orElse and subCommands

(cmd1 | cmd2).subCommands(???)
cmd1.subCommands(cmd2).subCommands(???)

@pablf
Copy link
Member

pablf commented Jun 13, 2024

@Kalin-Rudnicki The cli library parse and shows the help command in CliApp.scala I think, inside def run. Then it should also show which file options were used, if I remember well. This probably would be added also in method run of CliApp.

Regarding the design of Subcommand, I think both ways can be reasonable, although the actual one seems more flexible. It would allow to have things like:

val cmd1 = ???
val cmd2 = ???
val cmd3 = ???
val cmd4 = ???
val cmd = (cmd1 | cmd2).subcommands(cmd3 | cmd4)

so you can use all the following commands:

cmd1 cmd3
cmd1 cmd4
cmd2 cmd3
cmd2 cmd4

I think that the best way to implement this feature given the situation is to parse the command and, once you know which one is the top-level command, to retrieve the file options. This or just to allow this feature for commands with an unique top-level command. Maybe @jdegoes wants to share his thoughts on this.

@Kalin-Rudnicki
Copy link
Contributor Author

Im going to pursue the unique top level option

@Kalin-Rudnicki
Copy link
Contributor Author

Does this seem reasonable, or do you have any recommendations of how to phrase it more clearly/concisely:

USAGE

  $ test-command --text <text>

  File options will parsed from files named '.test-command' in the following places:
    - current working directory
    - all parent directories of the current working directory
    - home directory


OPTIONS

  --text <text>
    A user-defined piece of text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants