This repository has been archived by the owner on Jan 20, 2019. It is now read-only.
-
-
Notifications
You must be signed in to change notification settings - Fork 54
Plugabble API for linting #15
Closed
jonathanKingston
wants to merge
1
commit into
ember-cli:master
from
jonathanKingston:pluggableLintOutput
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
- Start Date: 2015-05-19 | ||
- RFC PR: (leave this empty) | ||
- ember-cli Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Provide a unified API for linting for complete choice of testing framework and linting platform. | ||
|
||
# Motivation | ||
|
||
Allow user to have full control over what testing language and what linter they prefer. | ||
Currently ember-cli is tied into using JSHint always rather than being fully agnostic. | ||
|
||
# Detailed design | ||
|
||
In ember-cli addons main file. | ||
|
||
Expose a test generator API for the linting addon to call: | ||
``` | ||
lintTree: function(type, tree) { | ||
return jshintTrees(tree, { | ||
jshintrcPath: this.jshintrc.tests, | ||
description: 'JSHint ' + type, | ||
testGenerator: this.testGenerator | ||
}); | ||
} | ||
``` | ||
|
||
Expose a new test Generator hook that would return the lint errors transformed to the test language (This will likely be in the testing framework addon): | ||
``` | ||
testGenerator: function (relativePath, errors) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking the nicest would be defining an object shape for these which both JSHint and ESLint seem to do anyway.
I was thinking it would be nice if the objects extended from |
||
if (errors) { | ||
errors = "\\n" + this.escapeErrorString(errors); | ||
} else { | ||
errors = ""; | ||
} | ||
|
||
return "describe('JSHint - " + relativePath + "', function(){\n" + | ||
"it('should pass jshint', function() { \n" + | ||
" expect(" + !!errors + ", '" + relativePath + " should pass jshint." + errors + "').to.be.ok(); \n" + | ||
"})});\n"; | ||
} | ||
``` | ||
|
||
# Drawbacks | ||
|
||
- The API for all linters will restrict the API of testGenerator hook. | ||
- Errors in the linter not returning the correct API will cause issues in outputting. | ||
|
||
# Alternatives | ||
|
||
- Consider using native `new Error` for APIs into testGenerator. | ||
- Consider using object instead of multiple arguments for testGenerator. | ||
- Use blueprints instead of testGenerator. | ||
|
||
# Unresolved questions | ||
|
||
- What would the best API be for testGenerator? | ||
- How do we make ember handle multiple extensions of testGenerator? | ||
- Should we trigger a fatal error if testGenerator doesn't receive the valid API usage? | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make it linkTrees ? I would prefer to not have to lock us into a
mergeTree
only for linting.Alliteratively, we could just call
linkTree
multiple times if we get an array of trees.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason why
this.testGenerator
can't be called more than once; that was how I was expecting the blocking aspect to be resolved.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stefanpenner - We already have a
lintTree
hook, that is not part of the proposal. The proposal as I understand it, is more about passing thetestGenerator
through.@jonathanKingston - c/d?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the change was around the usage of the
this.testGenerator
API which other addons would define.