feat(detect-non-literal-fs-filename): change to track non-top-level require() as well
#105
Conversation
test/utils/import-utils.js
Outdated
|
|
||
| const { getImportAccessPath } = require('../../rules/utils/import-utils'); | ||
|
|
||
| const RuleTester = require('eslint').RuleTester; |
There was a problem hiding this comment.
Why are you using RuleTester to test a function in import-utils? Can't you just use Mocha tests here?
There was a problem hiding this comment.
It is possible to use Mocha without RuleTester. But that test would require parsing with espree, adding a parent field to each node, parsing the scope using eslint-scope, and emulating context.getScope(). I thought using RuleTester would be easier and more reliable testing than doing that, what do you think?
Or are you suggesting that I change the eslint rule for testing to run in eslint, and stop hacking the message validation?
There was a problem hiding this comment.
It's really confusing to be using RuleTester for something other than testing a rule. :) If you really need all of the ESLint infrastructure in place, then using the Linter object is a much clearer way to test this functionality.
rules/utils/import-utils.js
Outdated
| @@ -1,41 +1,196 @@ | |||
| module.exports.getImportAccessPath = getImportAccessPath; | |||
There was a problem hiding this comment.
Utility files should be outside of the rules directory.
There was a problem hiding this comment.
I hope to use it later in the detect-child-process rule as well. #104
So the utils directory is good for me.
There was a problem hiding this comment.
That's fine, but it shouldn't be in the rules directory. Please move it to the top level.
|
Thank you for the review. |
|
Oops, looks like they’re a conflict. Can you take a look? |
…detect-non-literal-fs-filename
|
I resolved the conflict by merging from |
This PR modifies the detect-non-literal-fs-filename rule so that it also checks for non-toplevel
require().I tried to do something similar to the changes made to detect-non-literal-fs-filename (#92) to detect-child-process (#104), but I noticed that detect-non-literal-fs-filename does not check for inline
require().With this change, instead of checking the top level node, it checks the scope variable to keep track of where it was defined. It's working fine as the test case shows, but the change was big 😓.