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

Initial implementation of general donejs add. #603

Merged
merged 6 commits into from Mar 31, 2016
Merged

Initial implementation of general donejs add. #603

merged 6 commits into from Mar 31, 2016

Conversation

m-mujica
Copy link
Contributor

This is branched of #600, I used the original implementation by @daffl with a few changes:

  • Added deprecation warnings to donejs init and donejs plugin

screen shot 2016-03-31 at 13 01 42
screen shot 2016-03-31 at 13 02 11

  • Added custom usage instructions to donejs add
    screen shot 2016-03-31 at 13 33 01

@daffl there is only one small difference with your impl, this one won't call init when the type is unknown, instead it'll complain about the invalid type value. What do you think? should I add that behaviour back? (a nicer diff 80c956e that does not include all of the refactor changes).

cc: @matthewp

Manuel Mujica added 4 commits March 30, 2016 17:40
The binary has a different name in windows, so I changed the test to
create the fake binary with the right name based on the OS it's running,
also this PR changes cmd-init to it re-uses the run-binary module which
has logic to handle the binary names based on the OS.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 80c956e on donejs--add into * on master*.

return runBinary(['init', folder], options);
};

function isValidType(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

JSHint will complain if functions are used before they are declared. We probably want to move this one to the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say jshint is wrong then :)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a pet peeve of mine, too. Imagine if this would be 1000 lines of code and I'd skim through it I have no idea what this function does until I find it somewhere later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

See, I like this style for that reason, if you have a really long function I like seeing the return early in the function body, I think it makes it easier to understand how it flows. Like:

function doSomeStuff(){
  var a = 'foo';
  var b = 'bar';

  return doOtherStuff();

  function doOtherStuff() {
    // this is a super long function
  }
}

@matthewp
Copy link
Contributor

What do you think? should I add that behaviour back?

Maybe I'm not following, but when within an existing donejs app you are able to do stuff like:

donejs add cordova

Which will install and run donejs-cordova, are we removing that ability?

@daffl
Copy link
Contributor

daffl commented Mar 31, 2016

No, @justinbmeyer wanted donejs add to be used consistently. So additionally to the old functionality (donejs add component, donejs add cordova etc.) you can now initialize a new app/plugin/generator via:

donejs add app|plugin|generator [foldername]

This is in place of

donejs init [folder] # donejs add app [folder]
donejs plugin [folder] # donejs add plugin [folder]
donejs init [folder] --type generator # donejs add generator [folder]

@daffl
Copy link
Contributor

daffl commented Mar 31, 2016

Nice work! 👍

@matthewp
Copy link
Contributor

So additionally to the old functionality

Yeah, I understand that, but looking at this change it looks like this breaks the old functionality. Maybe I'm misreading it, but won't this throw if you did donejs add cordova ?

@daffl
Copy link
Contributor

daffl commented Mar 31, 2016

Good catch. @m-mujica This needs to be a fall-through like it was in the original implementation (see https://github.com/donejs/donejs/blob/donejs-add/bin/donejs#L62) and I don't think it is right now.

@m-mujica
Copy link
Contributor Author

@daffl @matthewp I added support for the original donejs add command, made some changes to the add --help output to reflect the command variations:

screen shot 2016-03-31 at 15 44 29

It should good to go now :D

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5fbc1c1 on donejs--add into * on master*.

@matthewp
Copy link
Contributor

I have no further complaints, good work.

@matthewp
Copy link
Contributor

I take that back, it looks like place-my-order guide test is still not running... very weird.

@m-mujica
Copy link
Contributor Author

l5txiqe

@matthewp
Copy link
Contributor

So the PMO guide did run in the PR one but not in the push, don't want it to block this change, I'll take a look separately.

@m-mujica m-mujica merged commit 0abb614 into master Mar 31, 2016
@m-mujica m-mujica deleted the donejs--add branch March 31, 2016 20:26
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.

None yet

4 participants