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

[pjlinkdevice] Remove org.apache.common #14430

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Feb 18, 2023

  • Remove org.apache.commons

While some tests are provided and as far as i can determine all results returned for the subnet methods are identical to the apache lib, some corner cases might happen. I think that is inherent to implementing such method ourselves.

In totall 5 bindings use this import org.apache.commons.net.util.SubnetUtils so i could also prepare a PR for this util class to core repo, and re-use it from these bindings, otherwise, i duplicate this code to the other bindings too. @openhab/core-maintainers please advise

  • dscalarm
  • pjlinkdevice
  • pclogo
  • russound
  • zway

Signed-off-by: lsiepel <leosiepel@gmail.com>
Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel lsiepel requested a review from a team March 26, 2023 13:19
@lsiepel
Copy link
Contributor Author

lsiepel commented Apr 9, 2023

@nils are you available to review?

@nils
Copy link
Contributor

nils commented Apr 16, 2023

@lsiepel Thanks for the hint, somehow I did not notice your review request. Will check your change sometime next week, latest next weekend...

Are the other bindings using the same bits from org.apache.commons or all different? If they share a lot, I wonder how we can avoid duplicating the code (and if it's worth it, not saying that it's necessarily the case).

@jlaur jlaur added the additional testing preferred The change works for the pull request author. A test from someone else is preferred though. label May 9, 2023
@lsiepel
Copy link
Contributor Author

lsiepel commented Jul 29, 2023

@nils gentle ping, are you able to perform a review?

@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 15, 2023

@nils another gentle ping

@nils
Copy link
Contributor

nils commented Dec 26, 2023

Before this PR goes into its first anniversary, I finally managed to setup an environment in Docker with some (virtual) devices to find for the discovery. 🎆 🍾

image

Looks good to me from a functionality point of view, thanks for adding unit tests for the new class!
I should absolutely do that for the main functionality of this binding as well...

From an organizational point of view however, re-implementing this as copies in multiple bindings does not appeal to me very much.
As somebody responsible for this binding, I did not want to worry about this bit of functionality, thus I did use a library. With this change, not only me, but in the worst case also other binding maintainers will have to worry about very special, but reusable functionality. Also over time, the copies might develop each their own set of bugs and glitches.

In totall 5 bindings use this import org.apache.commons.net.util.SubnetUtils so i could also prepare a PR for this util class to core repo, and re-use it from these bindings, otherwise, i duplicate this code to the other bindings too. @openhab/core-maintainers please advise

I'd appreciate some additional thoughts from some @openhab/core-maintainers too!

@lsiepel
Copy link
Contributor Author

lsiepel commented Dec 26, 2023

Before this PR goes into its first anniversary, I finally managed to setup an environment in Docker with some (virtual) devices to find for the discovery. 🎆 🍾

image

Looks good to me from a functionality point of view, thanks for adding unit tests for the new class! I should absolutely do that for the main functionality of this binding as well...

From an organizational point of view however, re-implementing this as copies in multiple bindings does not appeal to me very much. As somebody responsible for this binding, I did not want to worry about this bit of functionality, thus I did use a library. With this change, not only me, but in the worst case also other binding maintainers will have to worry about very special, but reusable functionality. Also over time, the copies might develop each their own set of bugs and glitches.

In totall 5 bindings use this import org.apache.commons.net.util.SubnetUtils so i could also prepare a PR for this util class to core repo, and re-use it from these bindings, otherwise, i duplicate this code to the other bindings too. @openhab/core-maintainers please advise

I'd appreciate some additional thoughts from some @openhab/core-maintainers too!

Thanks for testing and confirming it works as expected. I’ll raise an issue at the core repo like I did with the string utils class to ultimately create a utility in core making the dependency on org.apache obsolete without creating duplicate code.

@lsiepel lsiepel added work in progress A PR that is not yet ready to be merged and removed additional testing preferred The change works for the pull request author. A test from someone else is preferred though. labels Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress A PR that is not yet ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants