-
Notifications
You must be signed in to change notification settings - Fork 3
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 support for linting .ts and .tsx files #7
Conversation
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 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.
👍
Mostly about the describe
parts
lib/resin-lint.coffee
Outdated
scripts = findFiles(['ts', 'tsx'], paths) | ||
errorReport = lintTsFiles(scripts, config) | ||
|
||
console.log(errorReport.output) |
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.
Do we need this?
On second thought, probably yes I guess...
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.
Yep we need this, this is how we display the result of the linting process to the end user
lib/resin-lint.coffee
Outdated
.boolean('u', 'Run unused import check') | ||
|
||
if options.argv._.length < 1 and not options.argv.p | ||
options.showHelp() | ||
process.exit(1) | ||
|
||
configPath = if options.argv.typescript then TS_CONFIG_PATH else CS_CONFIG_PATH |
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.
I think that this can move closer to config = parseJSON(configPath)
lib/resin-lint.coffee
Outdated
process.exit(errorReport.getExitCode()) | ||
|
||
if options.argv.typescript | ||
scripts = findFiles(['ts', 'tsx'], paths) |
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.
No string opinion on this, but would it make sense to abstract the content of the if
& the else
into separate functions?
I would find it great to have a single if
statement for the linting mode (argv.typescript
).
I'm thinking of having a runTsLint
& a runCoffeeLint
function that only accept paths
& options.argv.i
as options.
Internally they could call a getConfig(XS_CONFIG_PATH, 'Xlint.json', options.argv.i)
method that would handle the loading, merging and config override.
This way it would be easier if at some point we decide to extend this to run on multiple targets as well.
lib/resin-lint.coffee
Outdated
linter.lint(file, source, parsedConfig) | ||
return linter.getResult() | ||
|
||
|
||
module.exports = (passed_params) -> | ||
try | ||
options = optimist(passed_params) | ||
.usage('Usage: resin-lint [options] [...]') | ||
.describe('f', 'Specify a coffeelint config file to override resin-lint rules') |
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 are some occurrences of coffee
in these describe
statements.
lib/resin-lint.coffee
Outdated
|
||
CONFIG_PATH = path.join(__dirname, '../config/coffeelint.json') | ||
CS_CONFIG_PATH = path.join(__dirname, '../config/coffeelint.json') | ||
TS_CONFIG_PATH = path.join(__dirname, '../config/tslint.json') |
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.
Just an idea: How about using an object, containing the config filename (& possibly the target file types as well)?
@thgreasi Thanks for the feedback. I've tightened up the code and I think it will be much better suited to adding more languages in the future |
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.
👍 🎄 🎅 🎁
Connects to #3 change-type: minor
7c75951
to
faa7406
Compare
Connects to #3
change-type: minor