#2338: Add a new check commandlet - #2355
Conversation
…o 2338-basic-check-commandlet
Coverage Report for CI Build 32373870031Coverage increased (+0.04%) to 72.987%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions9 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
so thanks you for adding the basics for the new check commandlet, it looks good actually but i still have some issue that should be quick to fix |
| /** | ||
| * {@link Commandlet} to check the current repository for best-practices, starting from the current working directory (CWD). | ||
| */ | ||
| public class CheckCommandlet extends Commandlet { |
There was a problem hiding this comment.
I think CheckCommandlet should live directly in com.devonfw.tools.ide.commandlet like all the other commandlets (Build, Install, Status, …) every commandlet class is in that package today. The test is already there (CheckCommandletTest), so the main class would even sit next to its test again.
But i wil makes sure this match the convention
There was a problem hiding this comment.
Thanks for the review. I followed the existing structure used by CleanupCommandlet, which is also in a subpackage. However I am happy to move it if we want to place all commandletts in com.devonfw.tools.ide.commandlet.
| private void checkGitIgnore(Path repositoryRoot, List<CheckIssue> issues) { | ||
|
|
||
| Path gitignore = repositoryRoot.resolve(MissingGitignoreIssue.GITIGNORE); | ||
| if (!Files.exists(gitignore)) { |
There was a problem hiding this comment.
so here you do use java.nio for File existence check but we already have an intern class that can help you do so and stay consistent (fileAccess).
my suggestion use
fileAccess.isFile(gitignore)
| try { | ||
| lines = Files.readAllLines(gitignore); | ||
| } catch (IOException e) { | ||
| LOG.warn("Failed to read{}", gitignore, e); | ||
| return; | ||
| } |
There was a problem hiding this comment.
it will also help you avoid this try catch block because the IOException you try to catch here is already wrappt in readFileLines (instead of the Classic readAllLines) as IllegalStateException
my suggestion see:
readFileLines in FileAccessImpl
|
|
||
| boolean open = false; | ||
| for (CheckIssue issue : issues) { | ||
| IdeLogLevel.WARNING.log(LOG, "{}", issue); |
There was a problem hiding this comment.
you log this Issue two times here and at line 132, i think just one time will be okay
| } | ||
|
|
||
| /** | ||
| * Test that {@code ide check} reports a missing .gitignore and fails (exit code 0) without --fix. |
There was a problem hiding this comment.
just small improvment the description don't match the reality the exit code is 1 not 0
| } | ||
|
|
||
| /** | ||
| * @return the line number this warning refers to, or {@code null} if not applicable. |
| } | ||
|
|
||
| /** | ||
| * @return {@code true} if this warning can be automatically fixed via {@link #fix(IdeContext)}, {@code false} otherwise. |
| } | ||
|
|
||
| /** | ||
| * Attempts to fix the problem that caused this warning. |
| public boolean fix(IdeContext context) { | ||
|
|
||
| List<String> lines = new ArrayList<>(context.getFileAccess().readFileLines(getPath())); | ||
| lines.add(this.rule); |
There was a problem hiding this comment.
so two things here
first: an ArrayList is not really needed here because ReadFileLines is a List itself
second: you send return true here but if readFileLines() return null this will throw an NPE.
my suggestion here is to first check that lines are not null such as
List<String> lines = context.getFileAccess().readFileLines(getPath());
if (lines == null){
return false;
}
lines.add(this.rule);
context.getFileAccess().writeFileLines(lines, getPath());
return true;
you don't need to log it yourself because readFileLines already do it
|
@majesteSil Thanks for the review! I've applied your suggestions. |
|
@Hiepiscus |
3 participants
This PR fixes #2338
Implemented changes:
checkcommandlet..gitdirectory..gitignorefile in the repository root..gitignorerules:.*and!.gitignore--fix.checkcommandletTesting instructions
ide checkgit init) without a.gitignorefile and runide checkide check --fixand verify that.gitignoreis created containing.*and!.gitignore.*or!.gitignorefrom.gitignoreand verify that the issue is detected.ide check --fixand verify that the missing rule is added.Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc