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

Drop the cruft #225

Merged
merged 5 commits into from
May 27, 2024
Merged

Drop the cruft #225

merged 5 commits into from
May 27, 2024

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented May 27, 2024

Drop dependencies that are superfluous (one class used) or are deprecated:

  • drop commons-collections4
  • drop (to be deprecated) maven-dependency-tree

Slight updates and cleanup in dependencies.


https://issues.apache.org/jira/browse/MSHADE-473
https://issues.apache.org/jira/browse/MSHADE-474
https://issues.apache.org/jira/browse/MSHADE-475
https://issues.apache.org/jira/browse/MSHADE-476
https://issues.apache.org/jira/browse/MSHADE-477

Single class mildly used it. Simple replacement added.
@cstamas cstamas self-assigned this May 27, 2024
@@ -185,21 +181,21 @@
<artifactId>maven-dependency-tree</artifactId>
<version>3.2.1</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

? really needed? this doesn't look related to the subject of the PR

Copy link
Member Author

Choose a reason for hiding this comment

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

? unsure I get your comment. If you'd look carefully, you will see that I just moved this dependency from last place (as it is non test) to here. I just "moved" it to have proper dep order.

Copy link
Member

Choose a reason for hiding this comment

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

ah right. sorry for that.
aren't we using spotless for pom?

<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.13.0</version>
<version>2.16.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look related to the subject of the PR
maybe change the subject

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the subject

Copy link
Member

Choose a reason for hiding this comment

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

changed the subject

thanks for that. The new subject is really more comprehensive :P

@@ -79,12 +79,11 @@
</distributionManagement>

<properties>
<mavenVersion>3.6.3</mavenVersion>
<mavenVersion>3.9.7</mavenVersion>
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't look related to the subject of the PR
maybe change the subject

Copy link
Member Author

Choose a reason for hiding this comment

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

changed the subject

pom.xml Outdated
@@ -93,12 +92,12 @@
<dependency>
<groupId>org.eclipse.sisu</groupId>
<artifactId>org.eclipse.sisu.inject</artifactId>
<version>${sisu.version}</version>
<version>${version.sisu-maven-plugin}</version>
Copy link
Member

Choose a reason for hiding this comment

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

the property name doesn't look related to such library.
I know it's coming from the parent but this is really confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, is ugly, will figure out something. My point was really to not have "disconnected" the sisu version in this plugin vs parent.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't this inherited anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, seems it is, removing whole depMgt section and moving guice version to dep, and that's did it.

@cstamas cstamas changed the title Drop commons-collections Drop cruft May 27, 2024
@cstamas cstamas changed the title Drop cruft Drop the cruft May 27, 2024
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Lgtm.

@olamy olamy merged commit 199ffae into master May 27, 2024
49 checks passed
@mosabua mosabua deleted the drop-commons-collections branch May 27, 2024 21:27
@olamy
Copy link
Member

olamy commented May 27, 2024

@cstamas, sorry, my bad. I merged it instead of only approving it,

@cstamas
Copy link
Member Author

cstamas commented May 27, 2024

Np, this pr was "complete", so is ok to have it merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants