Skip to content

Conversation

fo-code
Copy link
Contributor

@fo-code fo-code commented Dec 7, 2021

An API which calculates the code differences - so called 'delta' - between two commits.
The resulting delta model contains information about:

  • which files have been changed in which way
  • which concrete lines within these files have been changed in which way

The implementation for Git is available in the downstream PR jenkinsci/git-forensics-plugin#390

@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #372 (c415577) into master (5a028ef) will increase coverage by 2.16%.
The diff coverage is 89.71%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #372      +/-   ##
============================================
+ Coverage     66.28%   68.44%   +2.16%     
- Complexity      307      365      +58     
============================================
  Files            41       48       +7     
  Lines          1053     1160     +107     
  Branches         88      100      +12     
============================================
+ Hits            698      794      +96     
- Misses          315      325      +10     
- Partials         40       41       +1     
Impacted Files Coverage Δ
...lugins/forensics/delta/DeltaCalculatorFactory.java 59.25% <59.25%> (ø)
...nkins/plugins/forensics/delta/DeltaCalculator.java 100.00% <100.00%> (ø)
.../jenkins/plugins/forensics/delta/model/Change.java 100.00% <100.00%> (ø)
.../plugins/forensics/delta/model/ChangeEditType.java 100.00% <100.00%> (ø)
...o/jenkins/plugins/forensics/delta/model/Delta.java 100.00% <100.00%> (ø)
...ins/plugins/forensics/delta/model/FileChanges.java 100.00% <100.00%> (ø)
...ns/plugins/forensics/delta/model/FileEditType.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a028ef...c415577. Read the comment docs.

Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

In general very well written!

Some methods are not yet clear to me but I did not yet read the downstream PR.

Comment on lines 49 to 59
/**
* Returns a {@link DeltaCalculator} for the specified {@link SCM repository}.
*
* @param scm the key of the SCM repository (substring that must be part of the SCM key)
* @param run the current build
* @param workTree the working tree of the repository
* @param listener a task listener
* @param logger a logger to report error messages
* @return a delta calculator for the SCM of the specified build or a {@link NullDeltaCalculator} if the SCM is not
* supported
*/
Copy link
Member

Choose a reason for hiding this comment

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

Somehow you did use a different coding style for JavaDoc comments. Didn't you open these projects using https://github.com/uhafner/warnings-ng-plugin-devenv? Then the coding style settings should be automatically set in IntelliJ.

Comment on lines 20 to 22
/**
* The {@link ChangeEditType} of the change.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Such comments are just duplicating the name and type of the field and should be omitted.

Comment on lines 18 to 20
/**
* The currently processed commit.
*/
Copy link
Member

Choose a reason for hiding this comment

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

See above, delete such comments

Comment on lines 23 to 25
/**
* The reference commit.
*/
Copy link
Member

Choose a reason for hiding this comment

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

See above

Comment on lines 28 to 30
/**
* The difference file created by Git.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Is this the content? OR in which format?

Comment on lines 19 to 25
private static Change change;

@BeforeAll
static void init() {
change = new Change(EDIT_TYPE, FROM_LINE, TO_LINE);
}

Copy link
Member

Choose a reason for hiding this comment

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

This makes the test hard to read, inline and use a simple factory method that returns the object in the test.

Change changeTwo = new Change(EDIT_TYPE, FROM_LINE, TO_LINE);

assertThat(change).isEqualTo(changeTwo);
}
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend to test equals in the following way:

   @Test
    void shouldObeyEqualsContract() {
        EqualsVerifier.simple().forClass(Change.class).verify();
    }

Comment on lines 23 to 29
private static Delta delta;

@BeforeAll
static void init() {
delta = new Delta(CURRENT_COMMIT_ID, REFERENCE_COMMIT_ID);
}

Copy link
Member

Choose a reason for hiding this comment

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

See above.

*
* @return the created map
*/
private Map<String, FileChanges> createFileChanges() {
Copy link
Member

Choose a reason for hiding this comment

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

Using an empty map would be sufficient , but it is fine to use the complex map here

Comment on lines 24 to 35
private static Map<ChangeEditType, List<Change>> changesMap;

private static FileChanges fileChanges;

@BeforeAll
static void init() {
ChangeEditType editType = ChangeEditType.INSERT;
List<Change> changes = Collections.singletonList(new Change(editType, 1, 6));
changesMap = new HashMap<>();
changesMap.put(editType, changes);
fileChanges = new FileChanges(FILE_NAME, FILE_CONTENT, FILE_EDIT_TYPE, changesMap);
}
Copy link
Member

Choose a reason for hiding this comment

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

See above. Maybe just inline the code and use EqualsVersifier for equals

@uhafner uhafner changed the title Code Difference API Add Code Difference API Dec 13, 2021
@uhafner uhafner added the feature New features label Dec 13, 2021
@fo-code fo-code requested a review from uhafner January 14, 2022 11:31
@fo-code fo-code marked this pull request as ready for review February 15, 2022 10:01
@uhafner uhafner merged commit 1e71a9c into jenkinsci:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants