[WIP] Feature/blueprint clone command by walreyes · Pull Request #133 · SpencerCDixon/redux-cli

walreyes · 2017-10-27T22:05:49Z

What does this PR do?

Adds blueprint clone <blueprint-name> <name> [options] command.
Solves copy or clone command for cli #108.

TODO

Review from maintainers.
Replace cloneTo path to the new settings.cloneTo
Review Blacklisted Files
Add Specs.

walreyes · 2017-10-27T22:06:45Z

Hey @anithri!

I have'nt added the specs because I first want to know if this looks good to you:

The Process I followed to add a new command:

I followed the same structure generate has:

I added the clone cli command under cli/cmds/clone.js and implemented a slightly different version of buildBlueprintCommands and buildBlueprintCommand (if the cli will have more commands these 2 classes could be refactored to be able to build different subcommands).
Added the Sub-Command Clone class and CloneBlueprint task.
Added a new model BlueprintCloner to clone the blueprint files into a new destination.

Each commit has a more comprehensive explanation of what each added class does.

Questions

Is there a correct way to get the cloneTo path right now?
I am doing: settings.blueprints.searchPaths[0] but I'm not sure if it's correct.

Why is this change necessary? For the clone command there needs to be a model to clone files. How does this address the issue? BlueprintCloner is initialized with a blueprint and some options. Then it implements a `clone()` method that will clone all the blueprint files. It ignores blacklisted files (.ds_store, .git, .gitkeep))

Sub-command Clone simplements a `run(blueprintName, args)` that simply calls the task `CloneBlueprint` to clone a blueprint.

CloneBlueprint is initialized with the blueprint that needs to be cloned and some options for the destination blueprint. CloneBlueprint calls BlueprintCloner to clone the files

Implements a slightly different version of build-blueprint-commands and build-blueprint-command. If the cli will have more commands it might be a good idea to make those two files adaptable to different descriptions, usages, etc.

codecov-io · 2017-10-27T22:19:07Z

Codecov Report

Merging #133 into master will decrease coverage by 7.36%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #133      +/-   ##
=========================================
- Coverage   82.17%   74.8%   -7.37%     
=========================================
  Files          27      28       +1     
  Lines         387     389       +2     
=========================================
- Hits          318     291      -27     
- Misses         69      98      +29

Impacted Files	Coverage Δ
lib/tasks/git-pull.js	`14.28% <0%> (-3.11%)`	⬇️
lib/tasks/create-and-step-into-directory.js	`14.28% <0%> (-0.72%)`	⬇️
lib/cli/cmds/generate/handlers.js
lib/models/blueprint-cloner.js	`2.85% <0%> (ø)`
lib/tasks/clone-blueprint.js	`0% <0%> (ø)`

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 00684d6...995135e. Read the comment docs.

anithri

This looks good so far. Most of the really big issues are due to upstream api's not being finalized yet.

anithri · 2017-10-29T19:52:58Z

src/models/blueprint-cloner.js

+const FILES_BLACKLIST = ['.ds_store', '.git', '.gitkeep'];
+
+export default class BlueprintCloner {
+  constructor(blueprint, options) {


let's use constructor(blueprint, options = {}) instead of line 12

anithri · 2017-10-29T19:53:26Z

src/cli/cmds/clone.js

+
+module.exports = {
+  command: 'clone <blueprint> <name>',
+  aliases: ['g', 'gen'],


probably the wrong aliases

indeed... aliases are optional so you can set an empty array or remove the key entirely.

Thanks!

I've fixed it.

anithri · 2017-10-29T19:59:16Z

src/cli/cmds/clone.js

+      .usage(usage)
+      .option('dry-run', {
+        alias: 'd',
+        describe: "List files but don't generate them",


~~2 different quoting styles used in this line and last/next. If you install and run prettier, that will help with your formatting.~~

~~This is the only descepency that eslint finds is in switch statements. Prettier wants cases indented, and eslint wants them flush with the switch statement.~~

// NO export default function(state = defaultState, action) { switch (action.type) { default: return state; } } // YES export default function(state = defaultState, action) { switch (action.type) { default: return state; } }

~~And so I've been editing it back after i yarn run pretty~~

@anithri

Prettier would use the two different quoting styles, because the describe string has an embedded single quote. Eslint should already allow that (and complain if double quotes are used in any other circumstance). The rule for quoting was modified by a recent PR

"quotes": [2, "single", {"avoidEscape": true}],

I prefer the prettier formatting for switch, and not having any manual step to undo it! This can also be accommodated by eslint:

"indent": [2, 2, {"SwitchCase": 1}],

Suggest we make that change in .eslintrc

anithri · 2017-10-29T20:02:18Z

src/models/blueprint-cloner.js

+
+import { fileExists } from '../util/fs';
+
+const FILES_BLACKLIST = ['.ds_store', '.git', '.gitkeep'];


We definitely need a FILES_BLACKLIST. I'm just wondering if there's value to making it an rc option that would include what you have here as defaults.

It might be worth it 🤔

If we set the defaults it's always good to let the user add more customization.

But in what other circumstances will we need to blacklist files besides clone?

anithri · 2017-10-29T20:05:13Z

src/models/blueprint-cloner.js

+  }
+
+  clone() {
+    const blueprint  = this.blueprint;


By the time a blueprint is added to the collection, it will already have a validated existent path to itself. And will also have a method that returns it's files.

Awesome!

I'll make the changes once the new api is ready.

anithri · 2017-10-29T20:14:13Z

src/models/blueprint-cloner.js

+    const blueprint  = this.blueprint;
+    this.ui.writeInfo('cloning blueprint...');
+
+    const cloneToDirectory = this.cloneToDirectory();


The directory we clone to will be determined at the settings level, something like rc.get('path.bpTarget). So by this point in execution, you'll either have a falsy value or a validated, existing path you can write to. Falsy values should generate a warning or an error.

It seems to me that making assumptions based on the search path order, is risking unintended consequences. Better to get 'permission' with an explicity defined path.

I totally agree, it is risky to make assumptions based on the search path order and it does make sense to get that permission from the explicit path.

I was aware that I will need to change the cloneDirectory to something like rc.get('path.bpTarget').

I will make the change once the new rc api is ready.

anithri · 2017-10-29T20:17:12Z

src/models/blueprint-cloner.js

+    return path.resolve(settings.blueprints.searchPaths[0], this.newBlueprintName());
+  }
+
+  newBlueprintName() {


I think this is another thing that should be from rc, so that we can add --name <newName> or --cloneTo or similar.

This is a great idea.

Let me check if I am understanding correct, rc will have:

All the settings from the .blueprintrc.

The UI environment to write.

The arguments sent from the CLI.

anithri · 2017-10-29T20:19:17Z

src/tasks/clone-blueprint.js

+import Task from '../models/task';
+import BlueprintCloner from '../models/blueprint-cloner';
+
+export default class extends Task {


Lets add an explicit class name, if only to make our IDE's happier.

FYI... none of the other Tasks currently specify a class name.

well, that's true. AirBNB recommend the existing way too:
https://github.com/airbnb/javascript#modules--prefer-default-export

TIL

anithri · 2017-10-29T20:39:16Z

src/cli/cmds/clone/build-blueprint-commands.js

+    _cloneDeep(bp[blueprint.name])
+  ));
+
+const buildBlueprintCommands = () => {


All of the blueprints will already be found, read, and have it's settings already merged. Again the api is not quite ready yet, but soon. And any suggestions would be most welcome. see #132 for current thinking

anithri · 2017-10-29T20:40:01Z

src/cli/cmds/clone.js

+  command: 'clone <blueprint> <name>',
+  aliases: ['g', 'gen'],
+  describe: 'Clone a blueprint with a different name',
+  builder: yargs => {


We'll want all commands to follow the same pattern, I'm working to finalize the api for that, but take a look at #132 for the current thinking. @jamesr73 ?

I'll take a closer look at this before providing final feedback... this seems to fall between the pattern used for simple yargs commands like init and the more involved pattern used to support the blueprint specific generate commands.

jamesr73

The clone logic looks good. The CLI needs handlers similar to:

// src/cli/cmds/generate

...
getHandler().onRun('generate', handlers.handleRun);
getHandler().onHelp('generate', handlers.handleHelp);
...

These are used by generate so it can parse argv in 2 stages, the first pass gets enough information to setup the environment and load the available blueprints, the second pass then runs the specific blueprint generate command, or outputs help.

Note that for the handlers to work generate.js module.exports does not include a handler property/function. And the builder property/function configures yargs with .exitProcess(false) and .fail(fn).

It's not clear that the clone command needs access to the blueprint specific options (yet?) so it could be a single command handler rather than needing to buildBlueprintCommands. I'm open to discussion on that.

I think the basic CLI interface would be:

bp clone <blueprint> [new_name]

Which would copy from wherever it is found on the search path to the local directory, optionally setting a new name. This would also support cloning a blueprint that is already in the local directory to a new name.

The other thing to consider is the help output:

bp help clone

jamesr73 · 2017-10-29T22:23:19Z

src/cli/cmds/clone.js

+  command: 'clone <blueprint> <name>',
+  aliases: ['g', 'gen'],
+  describe: 'Clone a blueprint with a different name',
+  builder: yargs => {


I'll take a closer look at this before providing final feedback... this seems to fall between the pattern used for simple yargs commands like init and the more involved pattern used to support the blueprint specific generate commands.

walreyes · 2017-10-31T17:30:27Z

Hey @anithri:

I see 3 important changes:

Change the cloneToDirectory to the one provided by the new rc
Add the --new-name and --clone-to options from the CLI
Remove blueprint files location logic and get those files from blueprint.files()

1 change that we need to discuss if it's worth changing:

Move blacklist files to rc and set defaults to: ['.ds_store', '.git', '.gitkeep'].

walreyes · 2017-10-31T17:30:47Z

Hey @jamesr73

I think you are right, clone it's not as complex as generate so it might not need the blueprint options. I don't think it needs them unless we think clone could do more besides cloning the blueprint files.

So I will make the change to not build all blueprint commands but only build the clone command
which will run the clone-blueprint task which will then look for the blueprint in the blueprint-collection and the send it over to the BlueprintCloner.

I am not sure yet how the generate handlers work 🤔 but it's only a matter of digging into the code to figure it out.

I'll refactor the clone command with the handlers.

walreyes · 2017-10-31T17:31:00Z

Thanks for the feedback @jamesr73 @anithri!

I will be making the changes I can without the new rc and then I'll add the remaining once it's ready.

jamesr73 · 2017-11-01T16:12:06Z

Hey @walreyes

The handlers are just callbacks registered with EventEmitters that are triggered after yargs has parsed the command line. See src/cli/index.js and src/cli/handler.js. This mechanism was added to support the rc api changes that are in progress. In particular it allows the yargs built in help support to be extended with information read from the settings.

Why is this change necessary? I used buildBlueprintCommands but `clone` does'nt really needs the blueprint options, with the name it's enough to clone the files How does this address the issue? Deletes clone/buildBlueprintCommands and uses handles to handle run

walreyes · 2017-11-07T17:25:01Z

@jamesr73 got it. So yargs emits an event with the command name and then the handlers pick them up and run the proper command. It's awesome!

I've refactored the cli/cmds/clone.js to a much simpler setup, a middle ground between init, config, new and generate.

What do you think?

jamesr73 · 2017-11-08T16:23:45Z

@walreyes looks good, much cleaner and clearer. I think we should also add a handler for help. Yargs will already provide it's default help for the command name, usage and options. The help handler should list the available blueprints. It doesn't need to use yargs to generate this, just log to the console (or use the ui).

jamesr73 · 2017-11-08T16:25:43Z

src/cli/cmds/clone/handlers.js

+
+export default handlers;
+
+function handleRun(argv, yargs, rawArgs = process.argv.slice(3)) {


The rawArgs parameter can be removed as it isn't needed here in the same way as it was for generate.

jamesr73 · 2017-11-08T16:27:49Z

src/cli/cmds/clone/handlers.js

+  const blueprintName = argv.blueprint;
+  const environment = getEnvironment();
+
+  if (blueprintExists(environment.settings.blueprints, blueprintName)) {


Pedant suggestion...

if (blueprintInEnvironment(blueprintName, environment) { ... }

walreyes added 4 commits October 27, 2017 17:18

Adds Sub-command Clone class.

d880b21

Sub-command Clone simplements a `run(blueprintName, args)` that simply calls the task `CloneBlueprint` to clone a blueprint.

Adds CloneBlueprint task.

636bb94

CloneBlueprint is initialized with the blueprint that needs to be cloned and some options for the destination blueprint. CloneBlueprint calls BlueprintCloner to clone the files

Adds clone cli command

e8ef3d1

Implements a slightly different version of build-blueprint-commands and build-blueprint-command. If the cli will have more commands it might be a good idea to make those two files adaptable to different descriptions, usages, etc.

walreyes force-pushed the feature/blueprint-clone-command branch from fb32cd3 to e8ef3d1 Compare October 27, 2017 22:18

anithri reviewed Oct 29, 2017

View reviewed changes

jamesr73 reviewed Oct 29, 2017

View reviewed changes

walreyes added 3 commits October 31, 2017 10:33

Solves clone wrong aliases

3b814a2

Runs pretty in blueprint-cloner.js

9069c7c

Defaults options = {} in BlueprintCloner constructor

ce2be51

jamesr73 reviewed Nov 8, 2017

View reviewed changes


		import { fileExists } from '../util/fs';

		const FILES_BLACKLIST = ['.ds_store', '.git', '.gitkeep'];


		export default handlers;

		function handleRun(argv, yargs, rawArgs = process.argv.slice(3)) {

Conversation

walreyes commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

TODO

Uh oh!

walreyes commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The Process I followed to add a new command:

Questions

Uh oh!

codecov-io commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

anithri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anithri Oct 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesr73 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walreyes commented Oct 31, 2017

Uh oh!

walreyes commented Oct 31, 2017

Uh oh!

walreyes commented Oct 31, 2017

Uh oh!

jamesr73 commented Nov 1, 2017

Uh oh!

walreyes commented Nov 7, 2017

Uh oh!

jamesr73 commented Nov 8, 2017

Uh oh!

walreyes commented Oct 27, 2017 •

edited

Loading

walreyes commented Oct 27, 2017 •

edited

Loading

codecov-io commented Oct 27, 2017 •

edited

Loading

anithri Oct 29, 2017 •

edited

Loading

jamesr73 left a comment •

edited

Loading