correct resolution of lint extends packages
#309
Conversation
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
==========================================
- Coverage 98.39% 98.01% -0.38%
==========================================
Files 32 32
Lines 561 555 -6
==========================================
- Hits 552 544 -8
- Misses 9 11 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #309 +/- ##
=========================================
+ Coverage 98.39% 98.4% +<.01%
=========================================
Files 32 32
Lines 561 563 +2
=========================================
+ Hits 552 554 +2
Misses 9 9
Continue to review full report at Codecov.
|
|
tests currently fail because |
|
Hey, thanks for this, I'll take a look at it tomorrow, it's late here now. Thanks for contributing 🎉🎉🕺🎉🎉 |
|
alright here we go. tslint config is really really simple, it turns out we were implementing a whole bunch of stuff tslint doesn't even support. the only thing it supports is so i went ahead and split it out into its own implementation. the tests have been cleaned up to be accurate to what tslint actually has too. |
|
Dude, <3 Thank you :) |
|
Would you like to be part of the “core team”? |
|
Sure, if you'd like. Would be happy to have a stab at some other issues when i'm free if needed. & no worries, let me know if there's anymore changes needed |
| .map(preset => resolvePresetPackage(preset, rootDir)) | ||
| .filter(preset => !path.isAbsolute(preset)) | ||
| .map(requirePackageName); | ||
| } | ||
|
|
||
| export default function parseTSLint(content, filename, deps, rootDir) { |
There was a problem hiding this comment.
Could you please paste this link somewhere around here?
There was a problem hiding this comment.
i added jsdoc linking to the docs instead as it explains there in friendlier terms what resolution is used.
| @@ -11,36 +11,20 @@ const testCases = [ | |||
| expected: [], | |||
| }, | |||
| { | |||
| name: 'detect specific plugins', | |||
| name: 'skip single built-in config', | |||
There was a problem hiding this comment.
I suppose the second one have covered this. We can remove this one.
There was a problem hiding this comment.
this is to test that you can use extends: 'foo' rather than an array, extends: ['foo']. so i think we should leave it here?
| 'tslint:recommended', | ||
| 'tslint:latest', | ||
| 'tslint:foo', | ||
| ], | ||
| }, | ||
| expected: [], |
There was a problem hiding this comment.
I am thinking about if we should declare tslint in expected array. I am not sure if there is a user case to declare the tslint.json but not script CLI command. I personally think it is worth to declare tslint here.
There was a problem hiding this comment.
just to clarify, does this mean the tslint parser would return ['tslint'] if deps doesn't contain it already?
|
@43081j The changes look good! Thank you very much for the contribution! BTW, I have invited you to the team. Welcome! |
There's currently a bug in master in that extending
plugin:@scope/nameshould result in@scope/eslint-pluginbut we incorrectly resolve it as@scope/eslint-plugin-nameor some such thing.In this PR, I have removed most of what logic we had around
extendsand (like ESLint does its self) have used the normalisation function I recently introduced.I also removed a bunch TSLint tests, because TSLint only supports loading inherited configs by node resolution (it just does a
requireon it). Because of this, I had to add a short-circuit in the resolution function to return the path as-is for tslint.cc @rumpl