Skip to content
This repository has been archived by the owner on May 20, 2020. It is now read-only.

Make global --json option explicit #40

Merged
merged 3 commits into from
Mar 21, 2019
Merged

Conversation

simonplend
Copy link
Contributor

@simonplend simonplend commented Mar 19, 2019

Having the --json option configured at a global level with yargs didn't feel particularly good in terms of making this project's codebase accessible to other developers ("where does the json option that this command uses actually come from?"), so I've followed the approach that Ebi takes, and each command now explicitly defines the options that it accepts.

Resolves #38.

@simonplend simonplend requested a review from a team March 19, 2019 16:57
@simonplend simonplend self-assigned this Mar 19, 2019
@simonplend simonplend added this to In progress in Build the foundations (v1.1.0) via automation Mar 19, 2019
.option("json", {
type: "boolean"
})
.group(['token', 'json'], 'Global Options:')
Copy link
Contributor

Choose a reason for hiding this comment

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

This is cool

@@ -7,7 +10,9 @@ const printOutput = require("../lib/helpers/print-output");
* @param {import('yargs').Yargs} yargs - Instance of yargs
*/
const builder = yargs => {
return yargs
const baseOptions = flow([withToken, withJson]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I read up on flow but I may not get it. Are these functions that are being called after you pass in the column and PR options?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like what would be the alternative way of doing this - without using lodash?

Copy link
Contributor Author

@simonplend simonplend Mar 20, 2019

Choose a reason for hiding this comment

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

Great question! The alternative would be:

return withJson(withToken(yargs))
	.option("column", {
		describe: "Project column ID",
		demandOption: true,
		type: "number"
	})
	.option("pull-request", {
		describe: "Pull request ID",
		demandOption: true,
		type: "number"
	});

Right now, this isn't terrible, but as we move options out into these shared helpers it would end up super nested e.g.

withAnotherThing(withSomethingElse(withSomething(withJson(withToken(yargs)))))

Lodash's flow method helps us out like this:

  • It accepts an array of functions e.g. const baseOptions = flow([withToken, withJson]);
  • It returns a function that we can pass an argument into e.g. baseOptions(yargs)
  • This function, when called, executes each function serially (one after another), taking the return value from the function it has just called and using that as the argument that it passes to the next function

In our case, our with* helper methods accept yargs as an argument and they also return that same object. return yargs.option(...) gives us the instance of yargs as yargs implements a fluent interface. You can see that in the code for yargs, the option() method returns self, which is the same object that the option() method itself is being defined on (self.option): https://github.com/yargs/yargs/blob/51876e69c71e9861fb09847530eeaec9be534f5f/yargs.js#L577-L687

This leads us to...

const baseOptions = flow([withToken, withJson]);

// Calling this function that `flow` has returned has the following effect:
// baseOptions(yargs)
//   -> withToken(argumentGivenTo_baseOptions)
//     -> withJson(returnValueFrom_withToken)

const result = baseOptions(yargs);

// `result` is the return value from `withJson`, the last function in the chain of functions
// that `flow` called. In our case here, that is `yargs`, so we can go and chain other
// `.option()` calls on to it, yay!

Choose a reason for hiding this comment

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

Is this directly replaceable with the proposed pipeline operator

Which would be

const result = yargs
	|> baseOptions
	|> withToken
	|> withJson;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@leggsimon Yep! Bring it on (although the syntax isn't the nicest). In this case it would actually be:

const baseOptions = yargs
	|> withToken
	|> withJson;

As baseOptions was just the identifier for the function returned by Lodash's flow method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that makes so much more sense and I totally understand why using flow is right for this situation! Thanks so much Simon, this was so friggin' helpful.

@simonplend simonplend marked this pull request as ready for review March 20, 2019 13:17
@simonplend simonplend merged commit 19e9419 into master Mar 21, 2019
Build the foundations (v1.1.0) automation moved this from In progress to Done Mar 21, 2019
@simonplend simonplend deleted the json-option-explicit branch March 21, 2019 10:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants