Review: move sorald to subfolder #779
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
|
I shall take a look at this on Monday :) |
MartinWitt
changed the title
algomaster99
reviewed
Apr 5, 2022
Comment on lines
-209
to
-233
| <plugin> | ||
| <groupId>org.jacoco</groupId> | ||
| <artifactId>jacoco-maven-plugin</artifactId> | ||
| <version>0.8.7</version> | ||
| <executions> | ||
| <execution> | ||
| <id>default-prepare-agent</id> | ||
| <goals> | ||
| <goal>prepare-agent</goal> | ||
| </goals> | ||
| </execution> | ||
| <execution> | ||
| <id>default-report</id> | ||
| <phase>test</phase> | ||
| <goals> | ||
| <goal>report</goal> | ||
| </goals> | ||
| <configuration> | ||
| <excludes> | ||
| <exclude>sorald/annotations/**/*.class</exclude> | ||
| </excludes> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Shouldn't dependenices like picocli and jacoco be in the top-level pom? I think they would be shared by all submodules?
Comment on lines
-209
to
-233
| <plugin> | ||
| <groupId>org.jacoco</groupId> | ||
| <artifactId>jacoco-maven-plugin</artifactId> | ||
| <version>0.8.7</version> | ||
| <executions> | ||
| <execution> | ||
| <id>default-prepare-agent</id> | ||
| <goals> | ||
| <goal>prepare-agent</goal> | ||
| </goals> | ||
| </execution> | ||
| <execution> | ||
| <id>default-report</id> | ||
| <phase>test</phase> | ||
| <goals> | ||
| <goal>report</goal> | ||
| </goals> | ||
| <configuration> | ||
| <excludes> | ||
| <exclude>sorald/annotations/**/*.class</exclude> | ||
| </excludes> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
Shouldn't dependenices like picocli and jacoco be in the top-level pom? I think they would be shared by all submodules?
There was a problem hiding this comment.
No only sorald core (the sorald subfolder) needs the CLI feature currently. JaCoCo could be added to top level, but then we would force all projects implementing the API to use JaCoCo. I'm unsure if this is a great result.
|
@MartinWitt I merged |
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.
As always, any perfect plan fails on some small things you forgot :D. Maven more or less needs a top-level parent for multimodule projects. I thought maven has the same features as Gradle for multimodule projects. This PR changes the sorald-parent to a top-level pom and moves all files from
/srcto a Sorald subproject. I split the PR in two commits on is only file movement and the other one is pom changes. Luckily, GitHub shows when a file is moved without changes. I believe the best way to review this is checkout the PR and have a look inside your IDE.