Multiple targets in lint runner #40
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PawanHegde
commented
May 28, 2019
PawanHegde
commented
May 28, 2019
| this.lintRegister = lintRegister; | ||
|
|
There was a problem hiding this comment.
Moved most of this logic to getAllFiles. Failing to create an instance because one of the paths is wrong is not very good. Also constructors should not have complicated logic.
PawanHegde
commented
May 28, 2019
| files.addAll(FileUtils.listFiles(file, null, true)); | ||
| } else { | ||
| // TODO: Add proper logging | ||
| Logger.getGlobal().warning(basePath + " is not a valid file path"); |
There was a problem hiding this comment.
Would love feedback here. We don't want to fail the runner because one of the provided paths was wrong. But it should be more conspicuous than a log. Is it possible to include this failure as a part of the report?
PawanHegde
commented
May 28, 2019
| for (LintRule lintRule : lintRegister.getLintRules()) { | ||
| try { | ||
| if (lintRule.getLevel() != LintLevel.IGNORE) { | ||
| Map<JSONFile, List<String>> lintReports = lintRule.lint(filesToLint.toArray(new JSONFile[filesToLint.size()])); | ||
| Map<JSONFile, List<String>> lintReports = lintRule.lint(filesToLint.toArray(new JSONFile[0])); |
There was a problem hiding this comment.
Better for performance. No change in semantics.
ankuagarwal
reviewed
May 29, 2019
ankuagarwal
reviewed
May 29, 2019
PawanHegde
force-pushed
the
multiple_targets_in_lint_runner
branch
from
1d87422
to
307aeb5
Compare
- Added Unit test - Make LintRunner not fail on creation if path doesn't contain valid JSON files
PawanHegde
force-pushed
the
multiple_targets_in_lint_runner
branch
from
307aeb5
to
dfa0ee8
Compare
ankuagarwal
approved these changes
Jun 3, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature
What is the current behavior? (You can also link to an open issue here)
Only one path can be provided to the LintRunner for linting. This could either be the path to a single file or a directory. But to lint multiple files/directory with the same rules, multiple instances of LintRunner would need to be created.
What is the new behavior (if this is a feature change)?
Multiple files or folders (or a combination) can be linted with a common set of rules by a single LintRunner.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Completely backward compatible
Other information: