Arguments aren't passed to run when using just 'danger'

[orta]

https://circleci.com/gh/danger/danger-js/324
https://circleci.com/gh/danger/danger-js/325

After doing

- yarn
- yarn add danger

I would have expected

yarn danger -- run --dangerfile dangerfile.circle.js or yarn danger -- -d dangerfile.circle.js to work. As that's what's in the code.

program
  .option("-v, --verbose", "Verbose output of files")
  .option("-c, --external-ci-provider [modulePath]", "Specify custom CI provider")
  .option("-d, --dangerfile [filePath]", "Specify custom dangerfile other than default dangerfile.js")
  .parse(process.argv)

but it didn't work:

yarn danger v0.18.1
$ "/home/ubuntu/danger-js/node_modules/.bin/danger" -d dangerfile.circle.js

  error: unknown option `-d'

weird

[alex3165]

When I install the latest version of danger, the executable JS file look like this:

program
    .version(package_json_1.version)
    .command("run", "Runs danger on your local system", { isDefault: true })
    .command("pr", "Runs your changes against an existing PR")
    .parse(process.argv);

There is nothing defined for a -d option in node_modules/danger/distribution/commands/danger.js

[orta]

Yeah, the debugging options have always been an ENV var

[macklinu]

It is strange that the TypeScript source shows the -d option in source/commands/danger.ts, but that line is not present in distribution/commands/danger.js, as stated in #177 (comment). I'm going to take a look at this and see if I can resolve this.

[macklinu]

Oops, I was looking at the wrong file. 😛 Still going to take a look.

[macklinu]

I wonder if this is related to tj/commander.js#469?

[paulmelnikow]

I just ran into this setting up Danger for Shields.

Would you be up for switching to argparse? It's a conscientious port of Python's absolutely delightful argument parser. It has a nice API, provides excellent error messages, and most importantly, does not allow errors to pass silently. I've used it in Node projects here and there and had good luck with it.

If not, no worries. I'm just not that invested in fixing commander.

[orta]

Love your work <3.

Yeah, I'm not committed to commander at all, I've not really figured out what's going on here ( as I consider our use case so vanilla too )

[paulmelnikow]

Thanks! My focus since joining Shields is scaling out contributions and I'm excited to get Danger into the mix.

I'll give this a shot when I've a second.

[paulmelnikow]

Seems like there are three ways to go about this:

1. Get commander to pass arguments to subcommands. Looks like this has been variously attempted: Fix option parsing with sub-commands. tj/commander.js#692 Inherit arguments to default subcommand tj/commander.js#614 Pass un-normalized unknown options to sub-commands tj/commander.js#612.
2. Stop using git-style subcommands, and instead make a unified runner using argparse. This seems cleanest.
3. Re-implement commander's git-style subcommands, in a not-broken way.

Thoughts?

[orta]

2, yeah

The expectations for args of each command is in an interface somewhere, so the arg parsing can just create an object which matches the same shape. Shouldn't be a hard refactor.

[paulmelnikow]

Is there a faster way to run the updated CLI than a full yarn build? On my machine it takes 7–10 seconds.

I tried yarn build:watch and it seems to respond after I save, but when I run node distribution/commands/danger.js pr hi my changes to the CLI don't seem to take effect. They do take effect after I run yarn build.

[paulmelnikow]

I'm not sure I understand this comment. It refers to dynamic properties, which must mean dynamic command-line arguments, though I can't see anywhere that a dynamic property of app is used.

[orta]

1, I don't think so - I very rarely use the build process, I run during dev time using ts-node: yarn ts-node source/commands/danger-init.ts

2, program has no type definition so I made an app which does have a type definition, it's not dynamic CLI args, but that I wanted to know what all the possible args are, you can see them here:

danger-js/source/commands/utils/sharedDangerfileArgs.ts

Lines 9 to 21 in e7ca5f2

	export interface SharedCLI extends program.ICommand {
	verbose: boolean
	externalCiProvider: string
	textOnly: boolean
	dangerfile: string
	}
	export default (command: program.ICommand) =>
	command
	.option("-v, --verbose", "Verbose output of files")
	.option("-c, --external-ci-provider [modulePath]", "Specify custom CI provider")
	.option("-t, --text-only", "Provide an STDOUT only interface, Danger will not post to your PR")
	.option("-d, --dangerfile [filePath]", "Specify a custom dangerfile path")

[paulmelnikow]

1, Super helpful. I didn't know you could do that. It's my first time using TypeScript. Thanks!
2, Yea, gotcha, I've been using those. I think not all of them apply to every command.

I have a first pass mostly done. It's a straight refactor, and does not fix the problem of args for plain danger.

I'll test it a little bit more and then PR it so you can take a look. Hoping you can help me test it also. I don't think the changes I'm making are covered with tests.

[orta]

Yeah, they won't be - I've always struggled with writing tests for the commands and opted to try move as much as possible outside of it. I'll give it a run when you've got something 👍

[orta]

I took a stab at the simplest answer to this, which is duplicating the code for running across danger and danger run in #447

[orta]

Didn't really fix it

[paulmelnikow]

Hey, sorry I didn't respond earlier. Let me push up the half-done work I have. If it seems in a promising direction I'll pick it back up.

Most of where I'm hung up at this point in the refactor is not understanding clearly which programs can accept which arguments. Some of them don't make sense for some commands, e.g. the command that takes a positional dangerfile shouldn't also accept one via flag. argparse is strict, so it's important to sort this out.

I think going from the working multi-program runner to fix this issue will be straightforward enough. Basically it'd mean detecting which of the two cases we're in, then applying the appropriate argument parser. The "running" of the command is manual (you'll see in the branch) so there's no magic.

[orta]

I'm happy to either make those decisions ( positional dangerfiles shouldn't happen etc ) - I'm also happy to have you have a fresh perspective.

As long as the runtime-y ones are consistent among: danger, danger run, danger pr, danger process I'm happy to make a major version change for a better perspective. Version numbers aren't precious to me.

[paulmelnikow]

Okay, sure, I could bring my perspective on it.

I'm not totally sure I understand what danger process is for. Which is the DSL being referred to here? What's the reason for doing the work in a separate process?

Also, is danger runner purely for internal use by danger? Is there a reason that needs to be invoked in a separate process? Also since that program is not user-facing at all, I wonder if it would make sense to split it out into a separate CLI.

[orta]

danger Process: http://danger.systems/js/usage/danger-process.html

TLDR: Let's other environments use Danger in their language without implementing all the hard stuff.

danger runner totally internal yes, it only exists to have the same infra as danger process Danger creates a "JSONDSL" which is passed to danger runner which then turns it into the real DSL in the reference. The process separation is important because it allows async code to work like you think, rather than like this

[orta]

So it needs to be able to be invoked via the OS somehow, adding an extra command just felt like the simplest answer

[orta]

Should be fixed in 3.0 which simplified all the CLI user interface