Issue #77: git: add line changes detection #82
Conversation
| .setOldTree(pair.commonAncestorTreeParser) | ||
| .setNewTree(pair.prTreeParser) | ||
| .call() | ||
| .stream() | ||
| .filter(entry -> entry.getChangeType() != DiffEntry.ChangeType.DELETE) | ||
| .map(DiffParser::convertDiffEntryToGitChange) |
There was a problem hiding this comment.
this can't be made into a lambda?
diff -> convertDiffEntryToGitChange(diff, formatter)
There was a problem hiding this comment.
Yes. I mentioned this at the post:
we have to use for-loop instead of map, for convertDiffEntryToGitChange now throws IOException
There can't be checked exception in lambda
There was a problem hiding this comment.
I didn't know this. We can ignore this for now.
| DiffEntry diffEntry, DiffFormatter formatter) throws IOException { | ||
| final List<Integer> lines = formatter.toFileHeader(diffEntry).toEditList().stream() | ||
| .flatMapToInt(edit -> Arrays.stream( | ||
| new IntRange(edit.getBeginB(), edit.getEndB() - 1).toArray())) |
There was a problem hiding this comment.
So this will convert 1,4 to 1,2,3?
| assertEquals("There should be 1 change detected", 1, changes.size()); | ||
| final GitChange expected = ImmutableGitChange.builder() | ||
| .path("HelloWorld") | ||
| .addLines(1, 2) |
There was a problem hiding this comment.
So these are the new/changed lines in the new file?
So we lost out on information like what lines were removed from old file? So complete removal of a line shows no line changes?
Won't we need to know lines being completely removed?
Please make 3 new tests to confirm this behavior: adding new line, and removing line, changing existing line.
There was a problem hiding this comment.
I split this UT to three, including adding line, removing line and changing line.
I added banned classes to import control. Please rebase. |
Rebased. |
| final List<GitChange> changes = DiffParser.parse( | ||
| repository.getDirectory().getParent(), "foo"); | ||
| assertEquals("There should be 1 change detected", 1, changes.size()); | ||
| final GitChange expected = ImmutableGitChange.builder() |
There was a problem hiding this comment.
As new test shows, removed lines are completely invisible from view.
This can stay for now, but I think it has to change as it will cause issues for some types of regression.
How will we know what method/code/comment was changed if no lines are displayed?
Issues that will be affected:
#6 - need to know which method
#49 - need to know if has change besides chmod
#69 - need to know if was comment or code
I think we have to work with both sides of the file and not just the latest file.
@Luolc Do you agree? If so, let's create a new issue, unless you want to try and fix it now.
There was a problem hiding this comment.
I split lines to addedLines and deletedLines now. Is that OK?
There was a problem hiding this comment.
I assume this should be ok.
Eventually we will probably need some way to examine the before and after file for it's contents and surrounding context.
| @@ -32,4 +34,18 @@ | |||
| * @return the path of the changed file | |||
| */ | |||
| String path(); | |||
|
|
|||
| /** | |||
| * The line numbers of the added codes. | |||
There was a problem hiding this comment.
What is code here? We can't be sure new lines are code, comments, etc...
Should this just be changes?
| List<Integer> addedLines(); | ||
|
|
||
| /** | ||
| * The line numbers of the deleted codes. |
| /** | ||
| * The line numbers of the added changes. | ||
| * The first line of a file is marked as line zero. | ||
| * @return the line numbers of the added codes |
| /** | ||
| * The line numbers of the deleted changes. | ||
| * The first line of a file is marked as line zero. | ||
| * @return the line numbers of the deleted codes |
#77
In method
parse, we have to use for-loop instead ofmap, forconvertDiffEntryToGitChangenow throwsIOException.Since
IntStream.rangeis banned for the cobertura bug, I use apache.common'sIntRangeinstead.I add a comment to ignore
ClassDataAbstractionCoupling.