Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Don't use glob with --project option #3313

Merged
merged 7 commits into from
Nov 28, 2017
Merged

Don't use glob with --project option #3313

merged 7 commits into from
Nov 28, 2017

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Oct 11, 2017

PR checklist

Overview of change:

Consistently uses absolute paths when dealing with ts.Program.
Don't use glob to find files, look them up in ts.Program instead.
Don't read files from disk, because they are already available as ts.SourceFile.
Fixes: #2584

Provide a better error message if a specified file is not part of the project.
Fixes: #2208

Is there anything you'd like reviewers to focus on?

Linter should get a new method lintFile or lintSourceFile. That can be done afterwards.

CHANGELOG.md entry:

[bugfix] better error message for files not contained in the project
[enhancement] avoid duplicate IO when using --project option
[deprecation] Linting non-existent files outputs a warning. This will be an error in TSLint 6.

@adidahiya adidahiya self-assigned this Oct 19, 2017
Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error for files not in the project is also thrown for non-existant files

How do you get to this state? By supplying a nonexistent filepath in "files" in tsconfig.json?

does not match the behavior without --project because glob simply ignores files that don't exist. -> throw an error there, too?

Yeah, I think I'd prefer this approach. Sounds like a serious misconfiguration of the linter if this happens.

src/runner.ts Outdated
files = files.map((f) => path.resolve(f));
filesFound = filterFiles(program.getSourceFiles().map((f) => f.fileName), files, true);
for (const file of files) {
if (!glob.hasMagic(file)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a little tricky; can you leave some comments to make this easier to follow? how do special characters in the pattern affect the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given a project that only includes index.ts.
If you run tslint -p . '*.js' '*.ts' you only lint index.ts. *.js doesn't match anything but is ignored because it's a glob pattern ("has magic").
On the other hand if you run tslint -p . index.ts test/index.ts you get an error because test/index.ts is not in the project. That's because it doesn't have magic. With the current implementation it doesn't even matter if the file exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense; I meant to say -- can you leave a code comment

@adidahiya
Copy link
Contributor

I think a changelog entry would be useful too, especially for the #2208 fix. Also we could mention potential performance improvements since we're not doing redundant file reads?

@ajafff
Copy link
Contributor Author

ajafff commented Oct 20, 2017

The error for files not in the project is also thrown for non-existant files

How do you get to this state? By supplying a nonexistent filepath in "files" in tsconfig.json?

You can still pass arbitrary file names as arguments to the CLI. For a project that contains only index.ts you can execute tslint -p . index.ts foo.ts bar.ts
Before this PR those file names were handled as glob patterns. glob only returned the files that actually exist so we would never show an error about a file that doesn't exist.
Now throw an error for files that exist, but are not included in the project as well as files that do not exist.

@ajafff
Copy link
Contributor Author

ajafff commented Oct 20, 2017

RE: throwing error on non-existant files
I can implement this for both branches, with --project and without. But isn't that a bit too breaking? How about printing a warning now and throwing an error in the next major version?

@adidahiya
Copy link
Contributor

How about printing a warning now and throwing an error in the next major version?

yeah, that seems reasonable 👍

@ajafff
Copy link
Contributor Author

ajafff commented Oct 22, 2017

@adidahiya updated the PR description to add changelog entries, added warnings for non-existent files with and without --project and added tests.

@adidahiya adidahiya merged commit 048be95 into palantir:master Nov 28, 2017
HyphnKnight pushed a commit to HyphnKnight/tslint that referenced this pull request Apr 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove duplicate fs.readFileSync call Misleading error for file not in project
2 participants