Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Processors should be considered as rightful dependencies #82

Merged
merged 4 commits into from
Apr 12, 2021

Conversation

afillatre
Copy link
Contributor

fixes #76

@codecov
Copy link

codecov bot commented Apr 11, 2021

Codecov Report

Merging #82 (49bc9da) into master (4b13a20) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 49bc9da differs from pull request most recent head 756334d. Consider uploading reports for the commit 756334d to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             master     #82      +/-   ##
===========================================
- Coverage      6.00%   5.96%   -0.05%     
  Complexity       23      23              
===========================================
  Files            26      26              
  Lines           966     973       +7     
  Branches        133     130       -3     
===========================================
  Hits             58      58              
- Misses          896     903       +7     
  Partials         12      12              

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 4b13a20...756334d. Read the comment docs.

@afillatre
Copy link
Contributor Author

Side-effect on the artifactUsedClassesMap attribut should be removed in another commit, but I modified a method outside my scope to be sure that at least the right state was returned. Please tell me if you're OK with that.

@cesarsotovalero cesarsotovalero self-assigned this Apr 12, 2021
@cesarsotovalero
Copy link
Collaborator

cesarsotovalero commented Apr 12, 2021

Side-effect on the artifactUsedClassesMap attribut should be removed in another commit, but I modified a method outside my scope to be sure that at least the right state was returned. Please tell me if you're OK with that.

Hi @afillatre, I'm OK with the proposal. I agree the DefaultProjectDependencyAnalyzer needs some refactoring.

Comment on lines 225 to +232
private Set<Artifact> collectUsedArtifacts(Map<Artifact, Set<String>> artifactClassMap,
Set<String> referencedClasses) {
Set<Artifact> usedArtifacts = new HashSet<>();
// find for used members in each class in the dependency classes
for (String clazz : referencedClasses) {
Artifact artifact = findArtifactForClassName(artifactClassMap, clazz);
if (artifact != null) {
if (!artifactUsedClassesMap.containsKey(artifact)) {
artifactUsedClassesMap.put(artifact, new HashSet<>());
}
artifactUsedClassesMap.get(artifact).add(clazz);
usedArtifacts.add(artifact);
}
findArtifactForClassName(artifactClassMap, clazz)
.ifPresent(artifact -> artifactUsedClassesMap.putIfAbsent(artifact, new HashSet<>()));
}
return usedArtifacts;
return artifactUsedClassesMap.keySet();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for this refactoring!
There are too many null in the project :)

Comment on lines +26 to 47
@MavenTest
@DisplayName("Test that DepClean runs in a Maven project with processors")
void processor_used(MavenExecutionResult result) {
assertThat(result).isSuccessful().out()
.plain().contains(
"-------------------------------------------------------",
" D E P C L E A N A N A L Y S I S R E S U L T S",
"-------------------------------------------------------",
"USED DIRECT DEPENDENCIES [1]: ",
" org.mapstruct:mapstruct-processor:1.4.2.Final:provided (1 MB)",
"USED INHERITED DEPENDENCIES [0]: ",
"USED TRANSITIVE DEPENDENCIES [1]: ",
" com.fasterxml.jackson.core:jackson-core:2.12.2:compile (356 KB)",
"POTENTIALLY UNUSED DIRECT DEPENDENCIES [1]: ",
" com.fasterxml.jackson.core:jackson-databind:2.12.2:compile (1 MB)",
"POTENTIALLY UNUSED INHERITED DEPENDENCIES [0]: ",
"POTENTIALLY UNUSED TRANSITIVE DEPENDENCIES [1]: ",
" com.fasterxml.jackson.core:jackson-annotations:2.12.2:compile (73 KB)"
);
}

}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice that you included a proper IT!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a very hard time debugging the plugin though, maybe I'm doing something wrong... I was unable to catch a breakpoint in the core project.

With more ITs, this big assertion can be refined, though.

@cesarsotovalero cesarsotovalero merged commit 9e79780 into ASSERT-KTH:master Apr 12, 2021
@sonarcloud
Copy link

sonarcloud bot commented Apr 12, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@afillatre afillatre deleted the processors_support branch April 12, 2021 13:23
afillatre pushed a commit to afillatre/depclean that referenced this pull request Mar 31, 2022
Co-authored-by: Alexandre Fillatre <a.fillatre@primobox.com>
Co-authored-by: cesarsotovalero <cesarsotovalero@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Mapstruct processor dependency is deleted
2 participants