Skip to content
This repository has been archived by the owner on Dec 10, 2024. It is now read-only.

Allow ".lint-test" suffix to work along with ".jshint" #108

Merged
merged 3 commits into from
Mar 16, 2016

Conversation

mitchlloyd
Copy link
Contributor

Let's hold off on merging this one until I have a chance to test the integration in an app.

@rwjblue
Copy link
Member

rwjblue commented Mar 8, 2016

Changes look good...

@mitchlloyd
Copy link
Contributor Author

Ok, I've been able to test this all the way through with a real app, and it's working!

Note the addition of the ember-cli-qunit bump.

@mitchlloyd
Copy link
Contributor Author

@rwjblue I suppose a major release is safest, but I wonder if any other software is attempting to hook into the "Disable JSHint" feature by using this file extension. I am not a SemVer lawyer but I don't feel like this would have been considered a public API in the past.

I did a search through github to see if anyone was using targetExtension = 'jshint.js'. I came across the ember-cli-template-lint and I see it uses "template-lint-test.js" as its file extension. For that to work with this feature it would need to be template-lint.js and the logic to exclude these tests would need to change slightly.

Is ending with lint-test.js a better convention for this?

@rwjblue
Copy link
Member

rwjblue commented Mar 10, 2016

Is ending with lint-test.js a better convention for this?

Yeah, that does make a bit more sense. It will require us to change our mechanism for loading modules though (since the regexp for -test would match both).

@mitchlloyd
Copy link
Contributor Author

If you leave an untested addon under your pillow at night...

Changing this suffix to lint-test was more involved than I thought. Ending linting tests with -test means that we must use the addModuleExcludeMatcher feature form ember-cli-test loader, otherwise the fall back prototype.shouldLoadModule method will let these through, even when we "don't include" them with addModuleIncludeMatcher. In other words, ember-cli-test-loader thinks everything that ends in -test should be included, unless you add an excludeMatcher.

This time I made this change backwards compatible. Old jshint modules will continue to work.

I still want to test this in a real app before merging. I can do this tomorrow. Also it would be great to add /vendor to the list of directories that are watched for rebuilds.

@mitchlloyd mitchlloyd changed the title Use more generic "lint" over "jshint" Use more generic "lint-test" suffix over "jshint" Mar 10, 2016
var TestLoaderModule = require('ember-cli/test-loader');
var TestLoader = TestLoaderModule['default'];
var addModuleIncludeMatcher = TestLoaderModule['addModuleIncludeMatcher'];
const TestLoaderModule = require('ember-cli/test-loader');
Copy link
Member

Choose a reason for hiding this comment

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

  1. I prefer the "liberal-let" strategy as coined in madhatted.com/2016/1/25/let-it-be
  2. vendor/ is not processed by babel, so these will not get transpiled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought this was in the spirit of this article since we're importing something that is "class-like". Checking with the author :)

In any event, looks a case for good ol' var!

@rwjblue
Copy link
Member

rwjblue commented Mar 14, 2016

Left a small nit-pick, but this looks good.

@mitchlloyd
Copy link
Contributor Author

Ok, made that change to use var instead of const.

@mitchlloyd mitchlloyd changed the title Use more generic "lint-test" suffix over "jshint" Allow ".lint-test" suffix to work along with ".jshint" Mar 16, 2016
rwjblue added a commit that referenced this pull request Mar 16, 2016
Allow ".lint-test" suffix to work along with ".jshint"
@rwjblue rwjblue merged commit 3b1a855 into ember-cli:master Mar 16, 2016
@rwjblue
Copy link
Member

rwjblue commented Mar 16, 2016

Thanks @mitchlloyd!

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