ESLINTJS-49 Rule no-implicit-dependencies doesn't work
#4819
Conversation
ericmorand-sonarsource
force-pushed
the
ESLINTJS-49
branch
4 times, most recently
from
a1d19fb
to
ee44bdd
Compare
ericmorand-sonarsource
changed the title
no-implicit-dependencies doesn't work
ericmorand-sonarsource
force-pushed
the
ESLINTJS-49
branch
from
ee44bdd
to
8feea56
Compare
| @@ -20,11 +20,6 @@ | |||
| import { RuleTester } from 'eslint'; | |||
| import { rule } from './'; | |||
| import path from 'path'; | |||
There was a problem hiding this comment.
I think we should do the same for all rules calling loadPackageJsons in the unit tests
There was a problem hiding this comment.
I agree, but it would require another broader ticket. What do you think?
There was a problem hiding this comment.
yes, check my comment below, we can create tickets for all rules having same issue
| }, | ||
| ): Array<PackageJson> => { | ||
| // if the fileName is not a child of the working directory, we bail | ||
| const relativePath = Path.relative(workingDirectory, fileName); |
There was a problem hiding this comment.
seems qa in failing in windows, it may be due to backward slashes. Check other helpers where we always transform paths with toUnixPath helper
There was a problem hiding this comment.
Fair enough. The path module is OS-aware, though. If this fails, it means that at some point we are using explicit path separator.
There was a problem hiding this comment.
but some paths are sent from java. to be sure we are working with the proper paths, always use path.unix methods. Windows supports in any case forward slashes also
There was a problem hiding this comment.
I moved to path/posix.
| @@ -35,8 +37,19 @@ const messages = { | |||
| export const rule: Rule.RuleModule = { | |||
| meta: generateMeta(meta as Rule.RuleMetaData, { messages, schema }), | |||
| create(context: Rule.RuleContext) { | |||
| // we need to find all the npm manifests from the directory of the analyzed file to the context working directory | |||
| const manifests = getManifests(context.filename, context.cwd, fs); | |||
There was a problem hiding this comment.
I would like this logic to sit in the package-json helpers, I think other rules are affected by this
There was a problem hiding this comment.
You mean the rest of the code, the one that gather the dependencies? If so, I didn't want to add another getDependencies method there as there already is one and I don't feel comfortable changing things that are outside of the scope of the ticket.
There was a problem hiding this comment.
ok fair enough. can you please also check what other rules depend on having the package-json cache? just create tickets for those as well pls and we'll see how we prepare the helper
ericmorand-sonarsource
force-pushed
the
ESLINTJS-49
branch
6 times, most recently
from
af5cefa
to
0526a3e
Compare
|
would be nice to add an integration test so that we are sure the rule works now in eslint context |
ericmorand-sonarsource
force-pushed
the
ESLINTJS-49
branch
from
0526a3e
to
4dffaed
Compare
ericmorand-sonarsource
force-pushed
the
ESLINTJS-49
branch
from
4dffaed
to
f46aad5
Compare
|
Fixes ESLINTJS-49