-
Notifications
You must be signed in to change notification settings - Fork 581
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
Support Tslint 5 #2779
Support Tslint 5 #2779
Conversation
The test data for test_bad_alex_no_dead_link was causing a error about gender, which was unnecessary in this test. The underlying error has changed slightly in newer versions of the linter, so removing the test case allows the bear to pass tests on a wider array of linter versions. Closes #2777
tslint 5 requires a configuration file, which breaks test TSLintBearOtherOptionsTest and other tests. However, this test supposedly provides coverage for 'rules_dir', but it is provides '/' as the rules directory, which is invalid, and does not confirm this functionality is actually working. Replace with pragma no cover temporarily, until proper tests are written. Related to #1072
tslint 5 requires a configuration file, so provide a different config file for each test. Related to #1072
Updates the tslint dependency to be any version from v3 to v5, as the tests now work on them all. Closes #1072
@@ -37,7 +37,7 @@ def create_arguments(filename, file, config_file, | |||
args = ('--format', 'json') | |||
if tslint_config: | |||
args += ('--config', tslint_config) | |||
if rules_dir: | |||
if rules_dir: # pragma: no cover |
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.
Worth noting that since tslint requires a tslint.json
otherwise it does nothing if no rules are selected , rules_dir
is a bit redundant as tslint.json
will usually contain the rulesDirectory
.
It might be simpler to deprecated/decommission rules_dir, or at least not enhance it until someone does some analysis to show it is useful.
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.
If we don't provide rules_dir
option in the future, that means tslint_config
option becomes a must?
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.
If there is no value for tslint_config
, tslint may find tslint.json in the top level directory where coala is run. If there is no tslint.json, there are no rules being used, especially not any custom rules which might reside in a custom directory.
If there is a tslint.json
, why wouldnt it include rulesDirectory
? It seems silly to not use rulesDirectory
.
It would seem to me that a rules_dir
setting only makes sense if the bear also had a setting for custom rule names to enforce, in which case a directory to get the rules from is needed.
But more analysis may expose a reason for rules_dir
.
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.
LGTM 👍 Let's make rules_dir
a follow-up issue.
Created #2783 |
@gitmate-bot ff |
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
Automated fastforward failed! Please fastforward your pull request manually via the command line. Reason:
|
@gitmate-bot ff |
Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently |
Automated fastforward with GitMate.io was successful! 🎉 |
Sits on top of #2778