This is simply a peeve of mine, but I don't think that checking for -1, specifically, makes sense. Why not rule out any negative value (i.e. >= 0)? This specific API may be documented to return exactly -1 (I haven't checked), but many others are not.

My favorite is:

  return !!~path.indexOf(somevar); // Note: Overly cute hack. Do not use! :-)

Take your time.... I'll wait 😉

Note in JavaScript it is explicitly a result of -1 if not found (http://es5.github.io/#x15.5.4.7). Simply checking for < 0 seems less correct, in the unlikely event they add other negative values for other scenarios.

While I agree that the meanings of other negative values are unspecified, it seems borderline inconceivable that they could eventually be defined as success cases. Ruling them out now seems more forward (and slightly-non-compliant implementation) compatible.

From the point of readability path.indexOf(node_modulesFolderName) !== -1 sucks.

As with any double-negative, you're forced to pause and do some mental calculations: "ah, it's checking for position, then asserts that position is not good; no wait it asserts that it's not wrong, then proceeds — therefore this statement checks that the substring exists in the path string! Phew...."

Rules are academic. In the end what counts is cost of maintenance. Replacing !==-1 with >=0 improves readability and reduces possibility of bugs.

Personally, I would add a comment explaining why we exclude such files.

The existing function program.isSourceFileFromExternalLibrary probably does what you want here.

Comment added as suggested.

The program.isSourceFileFromExternalLibrary cannot be used as it only identifies a file as external library if it is found during the dependency resolution. Since the getTodoComments method checks the file individually it fails when an editor submits a node_modules file as a root file.

Is fileName already normalized by this point? If not, is sourceFile.fileName?

If neither, consider just normalizing the path in isNodeModulesFile.

Changed to use sourceFile.fileName as it is normalized. I have confirmed checking the source code.

I don't understand these last 2 tests. They seem to be verify the comment IS present, when based on the file names shouldn't they be NOT present? Or am I reading the tests wrong?

This last two tests creates some node_module files with Todos in them.

Since no "spans" have been added to those files, the line of code verify.todoCommentsInCurrentFile(["TODO"]); pretty much checks for 0 todos (no more, no less). This makes the test scenario successful when the service don't report any todos when checking node_modules files even if they contain some.

The way the test code reads is entirely unintuitive (not your fault, but the semantics of the test infra). Some clarifying comments in the test would be good for future readers, else looks good to me. Thanks!

The check is for /node_modules/ — would it apply in Windows with its backslash directory separators?

Would it be sensible to add a test for that?

What if the whole project is situated nested inside node_modules somewhere? Will all TODOs suddenly disappear?

/myDir1/node_modules/myOtherDirectory/myProjectRoot/src/blablaFile.ts

1. People do put temporary junk in their node_modules for the purpose of quick-checking someething and avoiding erroneous commit.

2. Additionally, in complex large projects there may be a legitimate need to keep projects in node_modules due to some process optimisation.

All in all, the check should only turn true if node_modules appear after the project root in full path, not before it.