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

Prevent an app name of test. #256

Merged
merged 1 commit into from
Apr 5, 2014
Merged

Prevent an app name of test. #256

merged 1 commit into from
Apr 5, 2014

Conversation

twokul
Copy link
Contributor

@twokul twokul commented Apr 4, 2014

Related to #232, initial PR by @rjackson.

  • prevents the application to be named test
  • introduces new helpers stubPath and stubBlueprint
  • refacators generate test
  • adds tests for init and new commands
  • changes npm test command to make sure we run unit/commands tests
  • adds an entry to CHANGELOG

@twokul
Copy link
Contributor Author

twokul commented Apr 4, 2014

var isEmberCliProject = require('../utilities/is-ember-cli-project');
var checkName = require('../utilities/name-check');
Copy link
Member

Choose a reason for hiding this comment

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

why name the local var the reverse of the filename (checkName vs name-check.js)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no specific reason tbh, I can rename it.

@rwjblue
Copy link
Member

rwjblue commented Apr 4, 2014

👍 - I like this (thank you for picking it up).

<soapbox>
I am not generally a big fan of co-mingling functionality changes, and whitespace/formatting changes that are not directly related to what is being added. It makes reviewing the PR harder (since there is much more surface area to review), and likely will delay merging. This is all just a general gripe, I think these changes are good.
</soapbox>

@MajorBreakfast
Copy link
Contributor

Do we really want to introduce this spaces code style? I'd rather have multi var declarations instead of all these spaces. If we add it though, then we should follow it everywhere and I'll add it to the style guide (ARCHITECTURE.md) in my other PR.

Related to #232, initial PR by @rjackson.

+ prevents the application to be named `test`
+ introduces new helpers `stubPath` and `stubBlueprint`
+ refacators `generate` test
+ adds tests for `init` and `new` commands
+ changes `npm test` command to make sure we run `unit/commands` tests
+ adds an entry to CHANGELOG
@twokul
Copy link
Contributor Author

twokul commented Apr 4, 2014

@rjackson your point is well taken and sorry that PR got a little messy. Whitespace changes are not directly related, agreed.

@MajorBreakfast we should definitely agree on code style. It's kinda of bad to enforce my code style preference onto the project. Let's get the docs out of the door this weekend and we document and agree on coding style.

@stefanpenner
Copy link
Contributor

@MajorBreakfast I'm a sucker for 1 var for line + OCD alignment.

stefanpenner added a commit that referenced this pull request Apr 5, 2014
@stefanpenner stefanpenner merged commit dc1f6d2 into ember-cli:master Apr 5, 2014
@twokul twokul deleted the prevent-test-name branch April 5, 2014 00:34
@MajorBreakfast
Copy link
Contributor

@stefanpenner Okay then, let's do it xD

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