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

[MPH-189] Get rid of commons lang3 dependency #114

Merged

Conversation

Giovds
Copy link
Contributor

@Giovds Giovds commented Jun 7, 2024

Following this checklist to help us incorporate your
contribution quickly and easily:

  • Make sure there is a MPH-189 filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without
    pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [MPH-XXX] - Fixes bug in ApproximateQuantiles,
    where you replace MPH-XXX with the appropriate JIRA issue. Best practice
    is to use the JIRA issue title in the pull request title and in the first line of the
    commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will
    be performed on your pull request automatically.
  • You have run the integration tests successfully (mvn -Prun-its clean verify).

If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.

To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@Giovds
Copy link
Contributor Author

Giovds commented Jun 7, 2024

This requires the DefaultArtifactVersion to drop commons-lang3 in the maven-artifact module. This was just recently done with https://issues.apache.org/jira/browse/MNG-8146 and PR #1564 in core.

After that version bump is required here

Edit: it could be acceptable to just keep the dependency from exclusions as it is only used in test scope and will be removed automatically once maven-artifact dependency is version bumped. So I removed the exclusions and enforcer rule.

@Giovds Giovds marked this pull request as ready for review June 7, 2024 15:19
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

Change looks ok ...

But we should figure out how to avoid calling private methods at all - of course it is another issue and PR

@Giovds
Copy link
Contributor Author

Giovds commented Jun 26, 2024

I agree.

The main reason I kept it, is because it's used only for test code which was already based on reflection. I didn't want to changing the access modifiers of the actual class. Like you said, I wanted to focus this PR on removing the dependency.

@michael-o michael-o self-requested a review July 9, 2024 20:21
@michael-o
Copy link
Member

Will try to check in a couple of days.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

You so often repeat the same lines that it would make sense to move that to a private method.

Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Good to squash and merge.

@slachiewicz slachiewicz added enhancement New feature or request dependencies Pull requests that update a dependency file labels Jul 27, 2024
@slachiewicz slachiewicz merged commit dfe5512 into apache:master Jul 27, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants