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

[neeo] Remove org.apache.common #14442

Merged
merged 7 commits into from
Oct 21, 2023
Merged

[neeo] Remove org.apache.common #14442

merged 7 commits into from
Oct 21, 2023

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Feb 18, 2023

  • Remove dependency on org.apache.commons

Untested, minor refactoring only from code perspective

Signed-off-by: lsiepel <leosiepel@gmail.com>
@lsiepel
Copy link
Contributor Author

lsiepel commented Apr 7, 2023

@tmrobert8 are you able to review?

@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 lsiepel added the work in progress A PR that is not yet ready to be merged label Aug 1, 2023
@morph166955
Copy link
Contributor

I can probably test this. I may be one of the last neeo users at this point.

@mherwege
Copy link
Contributor

mherwege commented Aug 1, 2023

I may be one of the last neeo users at this point.

You are not alone, still using it as well.

@morph166955
Copy link
Contributor

Awesome, glad to know I'm not the lone ranger.

@morph166955
Copy link
Contributor

@lsiepel What is the expected behavior that we are looking for to pass/fail this? I haven't looked at the code enough to see where the strings would be impacted with this. I'm loading the jar now.

@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 1, 2023

for to pass/fail this? I haven't looked at the code enough to see where the strings would be impacted with this. I'm loading the jar now.

I would call it succesfull if you managed to discover, add and control things. At that point about all changed lines have been called. Maybe use it for some days to see if anything strange happens.

@morph166955
Copy link
Contributor

Just to confirm, I'm only replacing the binding piece. Not the integration right.

@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 1, 2023

Just to confirm, I'm only replacing the binding piece. Not the integration right.

Yes.

@morph166955
Copy link
Contributor

I've dropped the jar in, it came up no errors so far. I'll use it for a few days to make sure it behaves as expected.

@morph166955
Copy link
Contributor

The binding looked good last night when using the remote. I didn't see any issues.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel removed additional testing preferred The change works for the pull request author. A test from someone else is preferred though. work in progress A PR that is not yet ready to be merged labels Oct 15, 2023
Copy link
Contributor

@morph166955 morph166955 left a comment

Choose a reason for hiding this comment

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

LGTM

@morph166955 morph166955 removed the request for review from tmrobert8 October 15, 2023 20:09
@morph166955
Copy link
Contributor

I'm fine with the proposed change assuming the dependencies on the core are obviously resolved.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@morph166955
Copy link
Contributor

@lsiepel Why are some of the imports pointing to the neeo StringUtils and some are pointed to the openhab core StringUtils?

@lsiepel
Copy link
Contributor Author

lsiepel commented Oct 17, 2023

@lsiepel Why are some of the imports pointing to the neeo StringUtils and some are pointed to the openhab core StringUtils?

Because not all methods are available in the core utility class. Only the methods that are shared across multiple bindings are moved to core.

@lsiepel lsiepel requested a review from a team October 20, 2023 17:23
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

Thanks! Two minor suggestions.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Copy link
Contributor

@jlaur jlaur left a comment

Choose a reason for hiding this comment

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

LGTM

@jlaur jlaur merged commit 38df4ac into openhab:main Oct 21, 2023
3 checks passed
@jlaur jlaur added this to the 4.1 milestone Oct 21, 2023
@lsiepel lsiepel deleted the neeo-apache branch October 21, 2023 20:52
querdenker2k pushed a commit to querdenker2k/openhab-addons that referenced this pull request Oct 29, 2023
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: querdenker2k <querdenker2k@gmx.de>
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
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.

4 participants