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

frint-cli: Introduce "new" command #325

Merged
merged 17 commits into from Nov 6, 2017

Conversation

@discosultan
Copy link
Contributor

commented Oct 2, 2017

About

Closes #220

Introduces the frint new command which will replace frint init in the future. For more information, see the issue above.

frint new <directory> --example <example>

where:

  • <directory> (optional) - name of the app folder to create. If not specified, will populate current working dir
  • <path> (optional, default=frintjs/frint/tree/master/examples) - GitHub repository path for example apps
  • <example> (optional, default=i.) - has two different shapes:
    1. <name> (default=counter) - name of the example to fetch from FrintJS official repository
    2. <organization>/<repository>/tree/<branch>/** - full path to arbitrary GitHub repository to fetch an example from

Testing

Can be tested running the following script:

mkdir test1
cd test1
frint new
cd ..
frint new test2
frint new test3 --example router
frint new test4 --example frintjs/frint/tree/frint-router/examples/router

The script will generate four test folders in the current working dir for various frint new scenarios.

discosultan added some commits Sep 29, 2017

@discosultan discosultan self-assigned this Oct 2, 2017

@discosultan discosultan requested review from mAiNiNfEcTiOn and Travix-International/core Oct 2, 2017

@coveralls

This comment has been minimized.

Copy link

commented Oct 2, 2017

Coverage Status

Coverage remained the same at 97.277% when pulling 4377d24 on impl-new-cmd into 0123345 on master.

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

please also update the README of frint-cli.

and also the root README of the repo under Quickstart.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

@fahad19 Before updating all the docs, is the command interface final? I mean mostly naming for params: path & example and using the structure --<param>=<value>.

@discosultan discosultan added the on hold label Oct 2, 2017

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

if --path is already present in the command, do we need an additional --example too?

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

The <name> option can be renamed as <directory> or something more descriptive to avoid any confusion. I first thought <name> is for example's name :)

@alexmiranda

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

I think both --name and --example are confusing flags in this context. If you're creating a new project, I suppose you want to determine the --template instead of the location/name of the example project it will download. Just my opinion but I'm ok with the current flags if you've had discussions on it already.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

if --path is already present in the command, do we need an additional --example too?

Currently they are separated in order not to always have to specify the full path with <example>. If you don't specify <path>, it will default to the official repo. Alternative would be to make <example> "smarter" to figure out itself whether it is defined as a full path or only an example name.

The option can be renamed as or something more descriptive to avoid any confusion. I first thought is for example's name :)

I agree only because all it currently does is create and name a directory. If it were to also transform package.json, for example, to name the application, then <name> would feel better. But this is something that can be changed in the future without compat issues. Therefore, I'll rename it to <directory> 😁.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

I think both --name and --example are confusing flags in this context. If you're creating a new project, I suppose you want to determine the --template instead of the location/name of the example project it will download. Just my opinion but I'm ok with the current flags if you've had discussions on it already.

Yeah, it's a bit confusing, because the intents of scaffolding a template vs setting up an example are sort of mixed. But previous frint init worked the same way, so I'm happy with keeping it the way it is.

@markvincze

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

I kinda like the way dotnet new works:

  • it always scaffolds in the current directory, but can be overwritten with --output
  • the name by default is the name of the directory, but can be overwritten with --name
  • the template to use is the only non-flag argument of the command
  • (and it has some additional flags, like --language, --auth, etc.)

So you can do:

  • dotnet new mvc: scaffolds an MVC app in the current directory with the default name. I like that this command looks really intuitive, and usually this is what you want.
  • dotnet new console --output ~/myapp --name foo --language f#: scaffolds an F# Console app in the folder ~/myapp, but uses the name foo

Not saying that we should do the same, just wanted to mention as an example.

@fahad19

This comment has been minimized.

@markvincze

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2017

Also with Yeoman, the name of the template (which I think is the most important thing) is a non-flag argument.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2017

@markvincze Yeah, that's the difficult part - there are so many different CLI schemes out there. This particular impl is based on a scheme used by Node Express framework CLI.

return;
}
streamFrintExampleToDir();
});

This comment has been minimized.

Copy link
@markvincze

markvincze Oct 2, 2017

Contributor

From line 69 to here it's duplicated also in the init command, maybe we can extract this code?

This comment has been minimized.

Copy link
@discosultan

discosultan Oct 2, 2017

Author Contributor

I currently opted not to extract any code, because AFAIK, frint init command will be deprecated in the future and its code removed. @fahad19 may correct me, if I'm wrong.

This comment has been minimized.

Copy link
@fahad19

fahad19 Oct 3, 2017

Member

yeah, init will be deprecated. in fact the deprecation message can be included in this PR too.

This comment has been minimized.

Copy link
@markvincze

markvincze Oct 3, 2017

Contributor

👍

@discosultan discosultan removed the on hold label Oct 3, 2017

@coveralls

This comment has been minimized.

Copy link

commented Oct 3, 2017

Coverage Status

Coverage remained the same at 97.277% when pulling 10c5dba on impl-new-cmd into 0123345 on master.

discosultan added some commits Oct 3, 2017

@coveralls

This comment has been minimized.

Copy link

commented Oct 3, 2017

Coverage Status

Coverage remained the same at 97.277% when pulling 61a527e on impl-new-cmd into 0123345 on master.

@coveralls

This comment has been minimized.

Copy link

commented Oct 5, 2017

Coverage Status

Coverage increased (+0.2%) to 97.432% when pulling b98f46d on impl-new-cmd into 0123345 on master.

@coveralls

This comment has been minimized.

Copy link

commented Oct 5, 2017

Coverage Status

Coverage increased (+0.2%) to 97.432% when pulling 7e8ce3a on impl-new-cmd into 0123345 on master.

Done!
Please run these two commands to start your application:

This comment has been minimized.

Copy link
@fahad19

fahad19 Oct 6, 2017

Member

depending on whether <directory> argument was given or not, we can also add an addition cd command.

Two scenarios:

1: with directory:

$ frint new myapp

Done!

Please run:

$ cd myapp
$ npm install
$ npm start

2: without directory

$ frint new

Done!

$ npm install
$ npm start

Sorry for nitpicking. Just trying to come up with as many use cases as possible :)

This comment has been minimized.

Copy link
@discosultan

discosultan Oct 6, 2017

Author Contributor

Sorry for nitpicking. Just trying to come up with as many use cases as possible :)

No worries, good feedback is good!

I was thinking about it actually or alternatively adjusting the wording to specify to run the following commands in the app folder.

const isCustomExample = example.indexOf('/') >= 0;
resolve(isCustomExample ? getContextForCustomRepo() : getContextForDefaultRepo());

function getContextForDefaultRepo() {

This comment has been minimized.

Copy link
@fahad19

fahad19 Oct 6, 2017

Member

if this function receives example as an argument, then it doesn't have to be defined inside a function. they can all be in the root:

function getContextForDefaultRepo(example) {
  return '';
}

function getOutputDirectory(yargs) {
  return '';
}

function getContextForCustomRepo(example) {
  return '';
}

function mapDepsToContext(deps) {
  return '';
}

This comment has been minimized.

Copy link
@discosultan

discosultan Oct 6, 2017

Author Contributor

Sure, I used nested functions more as a way of grouping rather than for making use of closures. It's a matter of preference, so it should be ideally consistent across the board.

@fahad19 , so you suggest to keep functions flattened within the file root and keep them as pure as possible?

This comment has been minimized.

Copy link
@fahad19

fahad19 Oct 6, 2017

Member

yes please. purer the better :)

@discosultan discosultan added the on hold label Oct 6, 2017

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 30, 2017

@discosultan: any update on this? :)

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2017

I need to compare it to appix to ensure interface similarity. Hopefully will get to it soon :)

@discosultan discosultan removed the on hold label Oct 31, 2017

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@fahad19 I think this one is good to go now.

  • Merged master
  • Included the requested changes regarding flattening functions
  • Improved the completion text shown to users
  • Tested new'ing in current dir, specified dir and from custom repo
@coveralls

This comment has been minimized.

Copy link

commented Oct 31, 2017

Coverage Status

Coverage increased (+0.009%) to 97.272% when pulling db97d2f on impl-new-cmd into 8e86224 on master.

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

@discosultan: awesome work! 🎉

this PR is gonna be highly impactful for beginners!

do you think it is wise to wait until we move to frintjs org before we merge this? the download URL can then point to the correct URL then.

let me know what you think.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2017

@fahad19 Sure, I don't see any reason to rush it. Let's make the switch to new org, then I will update the init and new commands and then we can merge it.

@fahad19

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

cool! 👍

Refs #355

@fahad19 fahad19 referenced this pull request Nov 2, 2017

Closed

Organization #355

@fahad19

This comment has been minimized.

Copy link
Member

commented Nov 6, 2017

@discosultan: can merge as soon as the URL is updated.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

@fahad19: will update after lunch

@coveralls

This comment has been minimized.

Copy link

commented Nov 6, 2017

Coverage Status

Coverage remained the same at 97.272% when pulling 4da40f0 on impl-new-cmd into f8cd1bd on master.

@coveralls

This comment has been minimized.

Copy link

commented Nov 6, 2017

Coverage Status

Coverage remained the same at 97.272% when pulling 8618ab5 on impl-new-cmd into f8cd1bd on master.

@discosultan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2017

All should be done now

@coveralls

This comment has been minimized.

Copy link

commented Nov 6, 2017

Coverage Status

Coverage remained the same at 97.272% when pulling 8618ab5 on impl-new-cmd into f8cd1bd on master.

@fahad19

fahad19 approved these changes Nov 6, 2017

@fahad19 fahad19 merged commit 6853958 into master Nov 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@fahad19 fahad19 deleted the impl-new-cmd branch Nov 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.