Skip to content
This repository has been archived by the owner on Nov 22, 2021. It is now read-only.

Adding options to allow composeWith() to work #1

Merged
merged 4 commits into from
Oct 7, 2016

Conversation

frozenskys
Copy link
Contributor

Added options to the main generator to allow use within other generators using the yeoman compose functionality. Also a minor fix to add a missing semi-colon in build.cake, and fix the entry point in the package.json

@agc93
Copy link
Contributor

agc93 commented Oct 7, 2016

Hey, thanks for the contribution! 👍

Your changes are failing linting for the reasons below:

   5:25  error  Block must not be padded by blank lines  padded-blocks
   6:1   error  Trailing spaces not allowed              no-trailing-spaces
  12:13  error  Use ‘===’ to compare with ‘null’         no-eq-null
  12:46  error  Expected '===' and instead saw '=='      eqeqeq
  18:13  error  Use ‘===’ to compare with ‘null’         no-eq-null
  18:44  error  Expected '===' and instead saw '=='      eqeqeq
  32:13  error  Use ‘===’ to compare with ‘null’         no-eq-null
  32:35  error  Expected '===' and instead saw '=='      eqeqeq

If you could update to fix these (try running npm run test to check for yourself) I'll merge it in and publish a new version straight away 😄

Let me know if you need a hand or if it's not clear.

@frozenskys
Copy link
Contributor Author

Hi, my master branch does indeed fail linting, but I thought I had created the pull request against the feature-composability branch which passes all tests on my development machine. As this is my first PR please can you check if I have created it against the correct branch - or do I need to merge it into master and submit the PR against that?

@frozenskys
Copy link
Contributor Author

Also I noticed that this change removes the yosay greeting in all cases, maybe it would be better to add it back in but only call this.greet(); if this.options === null || this.options === undefined ?

@agc93
Copy link
Contributor

agc93 commented Oct 7, 2016

Your quite right, I was running against the wrong branch! 😣

And yes, I agree regarding calling this.greet(): we obviously don't want to see it when composing (internally or from other generators), but when running "bare" I think it's a nice addition.

So if you can, add the greeting back in, ignore my previous comments and I'll happily merge in 👍

We should now only greet if no options are passed either on command line or via composeWith()
@frozenskys
Copy link
Contributor Author

Not sure that it's best practice but it works :-/

@agc93
Copy link
Contributor

agc93 commented Oct 7, 2016

Good enough for me @frozenskys 😆

@agc93 agc93 merged commit 2299c37 into cake-archive:master Oct 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants