Sub command opts #110

jamesr73 · 2017-09-24T14:36:41Z

Initial PR for discussion of #106.

Not intended for merge yet. Missing tests, docs, some potential functionality. Includes an update to default blueprint blueprint that isn't necessarily the final desired change, just a demo.

The bulk of the work is done in src/cli/cmds/generate.js, there are some comments in there worth discussing.

eslint barfs on blueprints/blueprint/files/blueprints/name/index.js, could use some help with that.

Sync with head

…options.

jamesr73 · 2017-09-24T14:47:31Z

jamesr73> bp
Usage:
  bp <command> [arguments] [options]
  bp help <command>

Commands:
  generate <blueprint> <name>  Generate project file(s) from a blueprint
                                                               [aliases: g, gen]
  init                         Initialize .reduxrc for the current project

Options:
  --help, -h     Show help                                             [boolean]
  --version, -V  Show version number                                   [boolean]

Please specify a command to execute

jamesr73> bp help generate
Usage:
  bp generate <blueprint> <name> [options]
  bp help generate <blueprint>

Blueprints:
  duck <name>       Generates a new duck
  blueprint <name>  Generates a blueprint with desired hooks       [aliases: bp]
  dumb <name>       Generates a dumb (aka Pure) component
  form <name>       Generates a connected form component using redux-form
  smart <name>      Generates a smart (aka container) component

Generate Options:
  --dry-run, -d  List files but don't generate them                    [boolean]
  --verbose, -v  Verbose output, including file contents               [boolean]
  --help, -h     Show help                                             [boolean]

jamesr73> bp help generate blueprint
bp generate blueprint <name>

Generate Options:
  --dry-run, -d  List files but don't generate them                    [boolean]
  --verbose, -v  Verbose output, including file contents               [boolean]
  --help, -h     Show help                                             [boolean]

Blueprint Options:
  --description, -D      override default description                  [boolean]
  --command, -c          add hook to specify blueprint command options [boolean]
  --locals, -l           add hook to specify locals                    [boolean]
  --file-map-tokens, -m  add hook for fileMapTokens                    [boolean]
  --before-install, -b   add hook for beforeInstall                    [boolean]
  --after-install, -a    add hook for afterInstall                     [boolean]

jamesr73> bp generate blueprint FizzBuzzer --dry-run --verbose --description --command --locals
   ::
  debug: fileContent: module.exports = {

  description: function() {
    return 'Generate from blueprint';
  },

  command: {
    aliases: [],
    builder: yargs => yargs
  },

  locals(options) {
    // Return custom template variables here
    return {};
  },
};

* master: Jest (SpencerCDixon#99) Refactor project-settings.js to use rc for .reduxrc (SpencerCDixon#100) # Resolved Conflicts: # .babelrc # blueprints/blueprint/files/blueprints/__name__/index.js # package.json

anithri

I like the way this is going, but it needs to adopt to standards we don't necessarily have yet.

anithri · 2017-09-24T19:17:21Z

.babelrc

@@ -1,8 +1,10 @@
 {
  "presets": ["es2015"],
+  "plugins": ["transform-object-rest-spread"],


I wholly approve of this amazing addition to JS.

anithri · 2017-09-24T20:05:33Z

blueprints/blueprint/index.js

@@ -1,13 +1,45 @@
 module.exports = {
  description() {
-    return 'Generates a blueprint template for creating new blueprints';
+    return 'Generates a blueprint with desired hooks';


We should devise a pattern for naming switches that toggle certain sections of blueprints on and off. Ditto for the file_tokens/paths. rc the new blueprintrc file aggragator. It allows options to be added via ENV variables using the format of blueprint_foo_bar_baz=42 which would resolve to {foo: {bar: {baz: 42}}} Maintaining that standard for options would be nice.

For demonstration sake, even if it's commented out, could you add blocks that show how to code both default and required values.

Sure. That's mostly yargs stuff, which I could link to in a comment, but I agree it serves as a useful example.

Upon investigation, it is also parsing command line args like so:
blueprint_bp__blueprint__afterHook=false bp config --bp.blueprint.command=true produces
{"bp":{"blueprint":{"afterHook": false, "command": true}}

We can turn off argv processing for rc and let yargs handle it. Can yargs allow arbitrary options? would I have to define '--bp.partials.includeDir=some/pathand--bp.partials.onlyPartials=true` explicity as options?

yargs can indeed allow arbitrary values. Or you can tell it not to. That could be decided/enforced on a command-by-command (blueprint-by-blueprint) basis.

I definitely agree that blueprint options and the rc files(s) need to be integrated. I'm open to discussion how that is best achieved. At the moment I'm leaning towards cleaner/simpler options in the blueprints themselves. If we offer the "full power" of yargs to blueprint authors it's unpleasant to mandate that they define them in --nested.dot.object format to map directly to the structure of the rc file(s). And makes them brittle if we change that format.

I've been thinking more along the lines of extracting the blueprint specific config from the resolved rc config and passing that to yargs (and maybe the options builder so it can relay them to the user). Possibly by setting a config property on the blueprint at load time. This would allow rc to handle environment variables to customise the nested structure but individual commands to specify simple options.

I'm more than happy to go with the consensus. Perhaps we could arrange a real-time discussion to get to the bottom of it?

anithri · 2017-09-24T20:17:56Z

src/cli/cmds/init.js

@@ -0,0 +1,9 @@
+import Init from '../../sub-commands/init';


Our goal for this should be to make it the same as bp generate blueprintrc --path=.

And to allow any blueprint to be able to use prompts like init does.

Interesting!

So we have a blueprint that covers the general options and dynamically interrogates individual blueprints for their options. And prompts the users for config values for everything.

I've definitely given some thought to interrogating the yargs config for blueprints such that init could dynamically generate the prompts for creating blueprintrc. I don't have a working solution for that yet.

Prompting the user for values for each options is likely something that would need to remain specific to an init implementation rather than having blueprintrc itself be a blueprint that could be generated the same as any other.

I'd say prompts them for missing but required values and/or for empty values of options that explicitly require a prompt.

Is it convenient that we could

.option('use-jsx', { alias: 'j', describe: "Save javascript files using .jsx", type: 'boolean', required: true, prompt: "JSX extension?" })

as for prompts remaining specific to init. I disagree. The only config that we need to generate that blueprint is the path the file is to be written too. It then prompts for options that aren't already designed. You make the init command require an argument or option that determine the path, which is passed to the constructor.

Forgive me, I'm not sure I have a clear understanding of your vision.

The idea is that any blueprint can prompt users interactively to provide required options that they haven't provided by means of rc/env/cli? Including one that generates blueprintrc?

To my understanding that is beyond the ability of yargs, although yargs can certainly define options as required and error out if they aren't provided. We can also ensure that values available via rc/env are given to yargs before it makes that decision.

Hmmm. Maybe I'm getting a handle on it.

blueprintrc is a blueprint that needs one required parameter for path. I can definitely put an example of that together.

It then prompts for a bunch of stuff, some of which it may be able to read from individual blueprints. Perhaps we could build that into the beforeInstall hook rather than yargs.

Probably a point of confusion in that I always meant this to happen at the Sub-Task level and not as part of yargs.

So that means required options would be handled by yargs, and prompts for options that are blank and needed by the blueprint.

Cool. Thanks. Then I completely agree. I'll look some more at how viable it is to extract the yargs options from each blueprint to feed them into prompts for init. Some initial investigation proved positive.

I may create a branch from my branch and play with that as a separate endeavour.

anithri · 2017-09-24T21:01:52Z

src/cli/cmds/generate.js

+      // opportunity here to interrogate yargs for configuration
+    }
+
+    // could do more to understand if options have been specified


This is my working theory for blueprintrc

{ "paths": { "smart": "containers", "dumb": "components", "blueprint": "~/.blueprints", "sourceBase": "sd", "testBase": "a", "fileCasing": "snake" }, "blueprintPaths": [ "/home/user/workspace/project/blueprints", "/home/user/.local/blueprints", "@someNppmNotSureAboutThis", "@butNeedSomeWay", "@toTellPathAndModule", "@apart" ], "defaults": { "git": { "add": true } }, "bp.store": { "sagas": false, "selectors": { "separateFile": "selectors.js" } }, "bp.blueprint": { "command": true } }

or {"bp": {"blueprint": {"command": true}}}

I touched on this earlier. I quite like format of:

options: { default: { propTypes: true }, blueprint: { command: true } }

And having the blueprint loader resolve the options for specific blueprints to keep them simple in the scope of an individual blueprint. It would be fairly simple, one time, logic.

Considering discussion on #101 how about:

bp: { common: { propTypes: true }, blueprint: { command: true } }

anithri · 2017-09-24T21:09:19Z

blueprints/blueprint/files/blueprints/__name__/index.js

-// beforeInstall: function(options) {},
-//   afterInstall: function(options) {},
-// };
+module.exports = {


All of the options here are controlling the presence of a chunk of the template. For the sake of clarity and until something more apt comes along, lets this a "Feature"

Features are going to require some presence in projectSettings. Either a boolean flag: show it or don't . The examples here are all booleans. More complicated objects can be anything that json represent. If you called command like bp gen blueprint --command.aliases=foo,bar that means you are going to display that feature, and you have some data meant just for it. and it should be in {"bp": {"blueprint": {"aliases": "foo,bar"}}}

bp as the leader there is to keep the top level from colliding between data for the cli, top level defaults, and data for individual blueprints. Any thoughts?

Agreed. The first stab at modifying the blueprint blueprint was just to show the new functionality working. The aliases option is an excellent one to show something more than a simple boolean. Not so good at showing a default in the rc file.

I could also imagine expanding the duck example to have more complex options. E.g. an array of types or multiple options that must be defined together (namespace/slice/types). I held off going down that route until we had an opportunity to discuss the basic implementation. I'm happy to look into that some more.

Expanding Duck and Form blueprints to allow options is required as much for being example of how it can be done as for the usefulness of the blueprint itself.

👍

I'll put together an example of duck options. Once we're happy with them we can look to build the ejs template(s) behind them. I'll leave the form blueprint changes to #5.

anithri · 2017-09-24T21:13:03Z

src/models/blueprint.js

@@ -93,6 +98,46 @@ export default class Blueprint {
    }
  }

+  static loadAll(options = {}) {


I get a little antsy with long files. I think we could separate the "find, load and instantiate" parts from the specific blueprint objects. blueprintLoader.js maybe?

Works for me. Just wanted a quick fix to get the cli piece working in the first instance.

Splitting/expanding functionality in the Blueprint may warrant a separate PR?

Yes, that is the best thing to do. I just started commenting on things tagential to many different issues and it all got a but muddled.

anithri · 2017-09-24T21:36:41Z

blueprintrc discussion: #101

jamesr73 · 2017-09-24T22:20:09Z

Thanks for all the feedback @anithri 😄

jamesr73 · 2017-09-24T22:38:07Z

PS a casual user is not required to know or use any of the yargs customisation options, a simple blueprint will work exactly as it does today (the CLI will set it up as a yargs command automatically). A user can pass arbitrary options to a blueprint and use them as they wish. The idea behind this functionality is to allow core/module/shared blueprints to better document and control the options that they support.

I'll have to check but.... I'm pretty sure that any integration with default values in rc file(s) would also work for a basic blueprint that accepted options but didn't document them with custom command configuration.

jamesr73 · 2017-09-24T23:45:51Z

@anithri thanks again for great feedback and discussion on this and other issues. I'll get some updates into the PR and we can move forward from there.

anithri · 2017-09-25T00:33:34Z

Here's my takeaway for the major areas of discussion that affect yargs and option naming

for the 'smart' blueprint...

[settings.options.defaults, settings.options.smart, CLI options] will be merged with cli settings overriding everything else.
Allow undocumented options an the cli.
need a way to indicate nested keys in options. parent__child or parent.child, does yargs do this already? env reader uses ``blueprint__options__blueprint__test=falseto insert data in tosettings.options.blueprint.test` but env has different syntax limitations than cli does, so we don't need to have them be identical formats
bp new to be made invisible for now and revisited later, refer to Boilerplates and the new command #105
will attempt to be able to re-use yargs option config at the task level to enable blueprint prompts.
Worry about refactoring code untill after yargs implemented
further .blueprintrc to Ideas for new .blueprintrc file structure #101
further partials discussion to blueprint partials #103
further prompt discussion to Adding prompts to blueprints #111

jamesr73 · 2017-09-25T00:53:19Z

Thanks for the roundup.

settings vs CLI options - I will add my vision of this to the PR for further discussion
undocumented CLI options are already supported
yargs supports config by environment variables with a syntax very similar to rc. Docs suggest it's all upper-case. BP_NESTED__DOT__OPTION vs BP_OPTION will depend on how we choose to integrate settings with CLI, let's see how we like my implementation of that 😉
I will add an invisible new command to the PR
I'll do some more digging into extracting yargs config and add to the PR if possible
thanks on refactoring
agreed on keeping discussions on individual issues, apologies for any confusing cross-talk
I've barely considered partials, I'll try give that some more thought

In addition:

I'll modify the blueprint blueprint with different examples of how command line args could be used
I'll add more complex command line args to the duck blueprint (not backed by template changes until the options are validated)

anithri · 2017-09-25T01:09:14Z

it would be harder to ignore ENV values for rc. There's not an easy way to do it. I'm not even sure how to do it without implementing the rc command. but...

given a app name of blueprint. the rc file is .blueprintrc and it expect env variables in the blueprint_key__child=value and that is decided in code. yargs is looking for a different prefix?

…help new' and 'bp new' both work.

…anitizeOptions hints that another hook would be useful.

…d execution.

…lueprint definition)

jamesr73 · 2017-09-25T13:50:19Z

settings vs CLI options - I will add my vision of this to the PR for further discussion
confirmed undocumented CLI options are already supported
yargs supports config by environment variables with a syntax very similar to rc. Docs suggest it's all upper-case. BP_NESTED__DOT__OPTION vs BP_OPTION will depend on how we choose to integrate settings with CLI, let's see how we like my implementation of that 😉
I will add an invisible new command to the PR
I'll do some more digging into extracting yargs config and add to the PR if possible
I'll modify the blueprint blueprint with different examples of how command line args could be used
I'll add more complex command line args to the duck blueprint (not backed by template changes until the options are validated)

…s by the init command.

jamesr73 · 2017-09-25T17:06:29Z

yargs supports config by environment variables with a syntax very similar to rc. Docs suggest it's all upper-case. BP_NESTED__DOT__OPTION vs BP_OPTION will depend on how we choose to integrate settings with CLI, let's see how we like my implementation of that 😉
I'll do some more digging into extracting yargs config and add to the PR if possible
used => Options can only be specified as a plain object.
I'll add more complex command line args to the duck blueprint (not backed by template changes until the options are validated)

…xon#10.

…n be addressed.

…nting can be addressed." This reverts commit dd19ea9.

… addressed).

codecov-io · 2017-09-28T13:32:14Z

Codecov Report

Merging #110 into master will decrease coverage by 7.84%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
- Coverage    66.3%   58.45%   -7.85%     
==========================================
  Files          14       29      +15     
  Lines         273      568     +295     
  Branches        0       46      +46     
==========================================
+ Hits          181      332     +151     
- Misses         92      236     +144

Impacted Files	Coverage Δ
test/helpers/regex-utils.js	`100% <100%> (ø)`
lib/models/task.js	`100% <0%> (ø)`	⬆️
lib/models/ui.js	`100% <0%> (ø)`	⬆️
lib/util/fs.js	`100% <0%> (ø)`	⬆️
lib/util/text-helper.js	`100% <0%> (ø)`	⬆️
lib/config.js	`100% <0%> (ø)`	⬆️
lib/models/project-settings.js	`100% <0%> (ø)`	⬆️
lib/models/sub-command.js	`100% <0%> (ø)`	⬆️
lib/sub-commands/generate.js	`13.33% <0%> (ø)`
lib/cli/cmds/generate/build-blueprint-command.js	`100% <0%> (ø)`
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04d9c30...5d03716. Read the comment docs.

…cked)

…e when BlueprintCollection is available.

jamesr73 · 2017-09-29T16:10:00Z

@anithri this is ready for review.

The big sticking point is linting the updated blueprint blueprint. Which is an eslint+ejs issue rather than an error in the code. I haven't been able to persuade eslint to even ignore the file completely, so I would welcome some help here.

The CLI changes have 100% test coverage except for buildBlueprintCommands which is small and is the isolated piece that will need to change to take advantage of your BlueprintCollection changes.

I haven't written any docs yet, I thought I could do that in a separate PR after the repo rename to avoid big conflicts with your changes.

I haven't looked at updating the duck blueprint, I think that's better done in a different PR (which I'm happy to get involved in).

…escription

* 'master' of https://github.com/SpencerCDixon/redux-cli: Sub command opts (SpencerCDixon#110) Update readme.md

jamesr73 added 3 commits September 19, 2017 12:39

Merge pull request #1 from SpencerCDixon/master

f4a8a02

Sync with head

Add bp command with init and generate sub-commands using yargs.

ea88eb4

Update the default blueprint blueprint to demo the blueprint command …

b63cc6b

…options.

jamesr73 added 2 commits September 24, 2017 18:04

Merge branch 'master' of https://github.com/SpencerCDixon/redux-cli

88ec520

Merge branch 'master' into sub-command-opts

3393093

* master: Jest (SpencerCDixon#99) Refactor project-settings.js to use rc for .reduxrc (SpencerCDixon#100) # Resolved Conflicts: # .babelrc # blueprints/blueprint/files/blueprints/__name__/index.js # package.json

anithri reviewed Sep 24, 2017

View reviewed changes

jamesr73 mentioned this pull request Sep 24, 2017

Ideas for new .blueprintrc file structure #101

Open

jamesr73 added 5 commits September 25, 2017 09:48

Add option to specify command aliases to blueprint blueprint.

b356385

Add support for 'bp new', hidden from 'bp help' for 2.0 release. 'bp …

47abcff

…help new' and 'bp new' both work.

Add a shortcut option to blueprint blueprint. Separate function for s…

c4f3c92

…anitizeOptions hints that another hook would be useful.

Load rc settings into blueprints and use for both command creation an…

e2e6b91

…d execution.

Allow rc file to specify blueprint aliases (overriding those in the b…

3345f55

…lueprint definition)

Specify options as a plain object that can be interrogated for prompt…

fd36429

…s by the init command.

jamesr73 mentioned this pull request Sep 25, 2017

Adding prompts to blueprints #111

Open

jamesr73 added 12 commits September 27, 2017 14:39

Add src to jest roots so watch mode sees source code changes.

92c1c53

Improve handling of missing or incorrect commands, address SpencerCDi…

c1dccfd

…xon#10.

Update blueprint blueprint to latest functionality.

3603acc

Remove old CLI commands from package.json bin.

b82b2f8

Add eplilogue support to blueprint commands.

780e540

Yo-yo back to non-singleton yargs for testing.

635456a

Add CLI parser tests, main/root command only.

a2720e2

Exclude blueprint blueprint ejs template from eslint until linting ca…

dd19ea9

…n be addressed.

Revert "Exclude blueprint blueprint ejs template from eslint until li…

64ed17e

…nting can be addressed." This reverts commit dd19ea9.

Move eslint in posttest hook (at least until blueprint linting can be…

909c216

… addressed).

Add tests for init sub-command.

f6166f2

Lint tests, make eslint aware of jest global.

c3c3615

jamesr73 mentioned this pull request Sep 28, 2017

Create shared test suite for all Tasks and SubCommands to confirm they are complying with the blueprint contract #24

Closed

jamesr73 added 2 commits September 28, 2017 14:24

Add tests for CLI new command.

42d1af5

Lint CLI 'new' test.

2b65601

jamesr73 added 5 commits September 28, 2017 17:56

Split generate command into smaller pieces more easily tested (and mo…

5feb333

…cked)

Add tests for buildBlueprintCommand

188a665

Remove yargs argument from buildBlueprintCommands, simplify for updat…

3272698

…e when BlueprintCollection is available.

Add tests for CLI generate.

b4613ab

Lint CLI generate test. Argh.

064d9b9

This was referenced Sep 29, 2017

eslint quotes avoidEscape #119

Closed

fileMapToken refactoring #112

Open

Update blueprint blueprint --description to accept a string for the d…

5d03716

…escription

anithri merged commit 55d90dd into SpencerCDixon:master Sep 30, 2017

This was referenced Oct 1, 2017

Have a catch-all for commands that don't exist. Provide useful information on potential commands user CAN use #10

Closed

Unclear/Uncaught error if generator called without a name argument #96

Closed

jamesr73 added a commit to jamesr73/redux-cli that referenced this pull request Oct 1, 2017

Merge branch 'master' of https://github.com/SpencerCDixon/redux-cli

e18c2ff

* 'master' of https://github.com/SpencerCDixon/redux-cli: Sub command opts (SpencerCDixon#110) Update readme.md

jamesr73 deleted the sub-command-opts branch October 2, 2017 16:51

Sub command opts #110

Sub command opts #110

Conversation

jamesr73 commented Sep 24, 2017

jamesr73 commented Sep 24, 2017

anithri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesr73 Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anithri commented Sep 24, 2017

jamesr73 commented Sep 24, 2017

jamesr73 commented Sep 24, 2017 • edited Loading

jamesr73 commented Sep 24, 2017

anithri commented Sep 25, 2017

jamesr73 commented Sep 25, 2017

anithri commented Sep 25, 2017

jamesr73 commented Sep 25, 2017

jamesr73 commented Sep 25, 2017

codecov-io commented Sep 28, 2017 • edited Loading

Codecov Report

jamesr73 commented Sep 29, 2017

jamesr73 Sep 24, 2017 •

edited

Loading

jamesr73 commented Sep 24, 2017 •

edited

Loading

codecov-io commented Sep 28, 2017 •

edited

Loading