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

Add disable jscs option to qunit #63

Merged
merged 1 commit into from
Sep 11, 2015

Conversation

toddjordan
Copy link
Contributor

Fix for Issue https://github.com/dockyard/ember-suave/issues/62

I added a checkbox to qunit -> "Disable JSCS"
When checked, none of the Suave JSCS tests are run.

I added 2 cases for disabling:

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 11, 2015

I was thinking that this would use an exclude matcher when disabled. Since we already match the default case for inclusion, we only need to suppress when the nojscs flag is set.

I would also like to require those changes to the test loader (I'll get 'em merged and tagged tomorrow), so we don't need to maintain both paths forever. It seems reasonable to me, that if an app wants this new feature that they have to ensure they are using the most recent version of the test loader...

@toddjordan
Copy link
Contributor Author

Yeah, that makes sense. I'll go ahead and make it an exclusion matcher, and simplify the code to assume your update is in.

@toddjordan
Copy link
Contributor Author

ok, updated. will check it out against your changes tomorrow.


included: function(app, parentAddon) {
var target = (parentAddon || app);
this._super.included(target);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs to call

this._super.included.apply(this, arguments);

(the super.included call needs to be bound to this)


Also, do we need the massaging of arguments to determine target like this or can we just call super with the args we were called with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I just took que from ember-cli-qunit: https://github.com/ember-cli/ember-cli-qunit/blob/master/index.js#L38 but if that's not needed, I'd be happy to just call super. I can try it out and see.

@brzpegasus
Copy link
Contributor

Thanks! Just need a squash, por favor :).

Simplify code based on review comments

check for qunit, call fix super invoke on included

Remove trailing line breaks
@toddjordan
Copy link
Contributor Author

squashed! 👞

brzpegasus added a commit that referenced this pull request Sep 11, 2015
Add disable jscs option to qunit
@brzpegasus brzpegasus merged commit 7e2249a into DavyJonesLocker:master Sep 11, 2015
@brzpegasus
Copy link
Contributor

@toddjordan Thank you! I'll release in a little bit.

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.

3 participants