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

[WIP] Feature/blueprint clone command #133

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

Conversation

walreyes
Copy link
Contributor

@walreyes walreyes commented Oct 27, 2017

What does this PR do?

TODO

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

@walreyes
Copy link
Contributor Author

walreyes commented Oct 27, 2017

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:

  1. 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).
  2. Added the Sub-Command Clone class and CloneBlueprint task.
  3. 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.
@walreyes walreyes force-pushed the feature/blueprint-clone-command branch from fb32cd3 to e8ef3d1 Compare October 27, 2017 22:18
@codecov-io
Copy link

codecov-io commented Oct 27, 2017

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Collaborator

@anithri anithri left a comment

Choose a reason for hiding this comment

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

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

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

export default class BlueprintCloner {
constructor(blueprint, options) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!


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

Choose a reason for hiding this comment

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

probably the wrong aliases

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

I've fixed it.

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

@anithri anithri Oct 29, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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


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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}

clone() {
const blueprint = this.blueprint;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome!

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

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

const cloneToDirectory = this.cloneToDirectory();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

newBlueprintName() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

import Task from '../models/task';
import BlueprintCloner from '../models/blueprint-cloner';

export default class extends Task {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

TIL

_cloneDeep(bp[blueprint.name])
));

const buildBlueprintCommands = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@jamesr73 jamesr73 left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

jamesr73 commented Nov 1, 2017

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
Copy link
Contributor Author

walreyes commented Nov 7, 2017

@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
Copy link
Contributor

jamesr73 commented Nov 8, 2017

@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).


export default handlers;

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

Choose a reason for hiding this comment

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

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

const blueprintName = argv.blueprint;
const environment = getEnvironment();

if (blueprintExists(environment.settings.blueprints, blueprintName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pedant suggestion...

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

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

Successfully merging this pull request may close these issues.

4 participants