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

Use "jshint" directly for linting #32

Merged
merged 1 commit into from
Jan 2, 2017
Merged

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Jan 2, 2017

This PR removes mocha-jshint and ember-cli-jshint in favor of calling jshint directly from the command line.

  • mocha-jshint has no advantages unless the project has other tests that also use Mocha
  • ember-cli-jshint is most important for linting the app and addon folder, that this project doesn't have

In the future we should probably also transition to ESLint instead.

@Turbo87 Turbo87 requested a review from marcoow January 2, 2017 15:08
@marcoow
Copy link
Member

marcoow commented Jan 2, 2017

mocha-jshint has no advantages unless the project has other tests that also use Mocha

Shouldn't we be adding some mocha tests though? Right now we're testing the AST processors by asserting on the processed code in the tests but that's actually not a good approach as we're always stripping everything which is not actually what we want - see this test for example which would break if data-test-* attributes that are assigned to components were stripped as well.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Jan 2, 2017

Shouldn't we be adding some mocha tests though?

probably not the worst idea, yeah 😉

unfortunately there is currently not a good testing story for build-time addons. one example of what's possible is https://github.com/ember-cli/ember-cli-eslint/blob/master/node-tests/test.js

I'd still prefer to use vanilla JSHint though unless there are good reasons to have it included in Mocha?

@marcoow
Copy link
Member

marcoow commented Jan 2, 2017

I'd still prefer to use vanilla JSHint though unless there are good reasons to have it included in Mocha?

Yeah, guess that makes sense

unfortunately there is currently not a good testing story for build-time addons. one example of what's possible is https://github.com/ember-cli/ember-cli-eslint/blob/master/node-tests/test.js

Could we use https://github.com/tomdale/ember-cli-addon-tests? Of course its main goal was making FastBoot integration tests easier but I think it should just as well work for our use case?

@marcoow marcoow merged commit 2cd2d30 into mainmatter:master Jan 2, 2017
@Turbo87 Turbo87 deleted the jshint branch January 2, 2017 15:27
@Turbo87
Copy link
Collaborator Author

Turbo87 commented Jan 2, 2017

Could we use https://github.com/tomdale/ember-cli-addon-tests? Of course its main goal was making FastBoot integration tests easier but I think it should just as well work for our use case?

yeah, that might work. I've been meaning to look into that project, but haven't gotten around to it yet.

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

Successfully merging this pull request may close these issues.

2 participants