Call formatter once for all file results #1472
Conversation
| files = files | ||
| .map((file: string) => glob.sync(file, { ignore: argv.e })) | ||
| .reduce((a: string[], b: string[]) => a.concat(b)); | ||
| processFiles(files, program); |
There was a problem hiding this comment.
This change was meant to go in the first commit.
|
Will review this later, but I'm up for doing a major version bump soon and this could be a key part of that |
| // Empty | ||
| } | ||
|
|
||
| public lint(fileName: string, source: string, configuration: any = DEFAULT_CONFIG): void { |
There was a problem hiding this comment.
What do you think about still allowing a configuration on the IMultiLinterOptions interface? Then this function would fall back to that if no configuration was provided here, and lastly fall back to the default config.
There was a problem hiding this comment.
Another thing to consider here is that source is actually optional if a program was provided before (see #1096)
There was a problem hiding this comment.
I actually have an idea that this function should return the formatted output just for the file currently being linted. This way, if being used as part of a build, lint failures could start being displayed before all files are linted, and this is great when using a formatter designed for console output. The getResult() method can still exist as is, to provide a way to get all results at once
There was a problem hiding this comment.
I'm not particularly keen on extending the scope of this PR, but let me say this:
this function should return the formatted output just for the file currently being linted
This is exactly what the (Single)Linter did so far: Lint one file and return the (formatted) result.
Adopting this behavior would exactly contradict the intention of this PR: To separate linting from returning the formatted output (once for all files linted so far).
if being used as part of a build, lint failures could start being displayed before all files are linted
I'm afraid this would require adopting my second proposed solution, while this PR covers the first proposed solution. Otherwise this would be incompatible with any formatter that requires header and footer.
If you're using console output, you could still achieve this by instantiating one MultiLinter per file (which is what the (Single)Linter would do). Returning formatted output for each file and also once at the end would be redundant (and a waste of computation time, if you ask me).
The getResult() method can still exist as is, to provide a way to get all results at once
If the result wasn't formatted, I wouldn't mind, but as it is, this would be redundant work. In general, I agree that linting should be separated from the result which should be separated from the formatting, but this is out of the scope of this PR. If we change too many things at once, we will very likely introduce bugs.
Another thing to consider here is that source is actually optional if a program was provided before (see #1096)
You're right, it needs to be optional (unfortunately the API is already quite confusing and this will add to it).
There was a problem hiding this comment.
One tweak that should simplify this PR a small bit is that we can merge the MultiLinter code into Linter and get rid of (Single)Linter all together.
I'm not overly concerned about having streaming results by default, as you mentioned it's still possible to achieve by linting one file at a time. I don't desire to make formatters stateful to achieve this. Given this, we can leave lint and getResult as they are.
With those things off the table, I believe the remaining major issue is the API. Between CLI options, tsconfig.json options, the ILinterOption interface, and "configurations", things are somewhat confusing like you stated. For the sake of this PR and keeping it limited, I think this comment is still relevant, as well as the optionality of source. I think we should be able to leave other changes for a separate PR.
There was a problem hiding this comment.
Just saw your comment on architecture/API structure. I don't have time to think about it deeply tonight, but in general it looked good to me. I think the last paragraph of my comment above still holds for the scope of this PR though
There was a problem hiding this comment.
What do you think about still allowing a configuration on the IMultiLinterOptions interface?
Then this function would fall back to that if no configuration was provided here, and lastly fall back to the default config.
Actually, I would prefer if we could omit the #lint method's configuration parameter at all.
Currently, this (still untyped) configuration can contain a rules directory and a list of rules, but I wonder why these parameters should be specified for one particular file? It would therefore be cleaner to attach the whole configuration including options (by merging these) to the instance of the linter. If one needs to lint one file with specific options, a separate linter instance could still be used.
There was a problem hiding this comment.
One tweak that should simplify this PR a small bit is that we can merge the MultiLinter code into Linter and get rid of (Single)Linter all together.
Would you mind if we do this in another PR? This change would probably require a couple of other changes (including to the test setup), so I tend to think it would be more safe to separate this.
There was a problem hiding this comment.
Would you mind if we do this in another PR? This change would probably require a couple of other changes (including to the test setup), so I tend to think it would be more safe to separate this.
Yep, makes sense to me, that seems fine.
Actually, I would prefer if we could omit the
#lintmethod'sconfigurationparameter at all. It would therefore be cleaner to attach the whole configuration including options (by merging these) to the instance of the linter.
I'm fine with this as well - it seems like a reasonable breaking change to include as part of 4.0. We have to be careful and consider all the ways TSLint gets used - as parts of IDE addons, as part of build tools, from the CLI, etc. But I think your ideas are solid - an IDE plugin could create a new instance of the linter for linting each file one by one as it's open; a build tool can lint all files together; etc.
|
@jkillian Let me know if this PR needs any additional work done before you can merge it. |
|
@caugner Sorry for the delay, merged the PR |
Fixes #656.
I have extracted a
MultiLinterwhich allows linting a sequence of files in such a way that the behavior ofLinteris maintained without changing its interface by using the special case where the sequence of files includes exactly one file.At the next major version bump, the
Lintercould be replaced with theMultiLinter.