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

Add string utils to core #3738

Merged
merged 11 commits into from
Oct 15, 2023
Merged

Add string utils to core #3738

merged 11 commits into from
Oct 15, 2023

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Aug 1, 2023

The goal of this PR is to add some string utility methods to core. This way they are available to all the bindings and we can finally get rid of the remaining apache.common libs.

While doing so, it seemed many bindings make use of the same apache StringUtils methods. To prevent code duplication it might be better to get the most used ones merged into core.

For reference: openhab/openhab-addons#7722
I'll also create an issue and PR's to update the openhab-addons.

Closes: #3783

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

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

It feels a bit like "yet another string utils library", but I can understand why you want to bring this into core.
It is up to core maintainers to decide if this should be included.

I have reviewed the PR from a code perspective and can give some technical input.
Overall two points: null handling is not yet consistent, and a few comments regarding performance.

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

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Consistency regarding Null annotations to be discussed with maintainers. I follow your argumentation regarding the drop in replacement. Maybe a more consistent interface would be worth the effort and pay off in the future.

Signed-off-by: lsiepel <leosiepel@gmail.com>
@kaikreuzer
Copy link
Member

I'm fine with adding those utils here - as long as we do not end up with a whole library in the end. 😆
Wrt nullability: My personal preference for such util methods would be to allow null values everywhere. But I'm also fine with a different decision. But in general I'd agree with @holgerfriedrich that it would make sense to apply the same logic for all of them.

@openhab/core-maintainers What's your position wrt this PR?

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

lsiepel commented Sep 22, 2023

Both kai and holger suggested aligning the null handling, so i did. I hope we can get this merged so i can update the addon PR's to use this.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

Fine. Just added a few findings, mainly related to the examples given in javadoc.

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

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

Thanks for implementing all the review comments!

My last point is about this:
I have the feeling that calling two methods capitalizeWords() and capitalizAllWords() is not right way. I assume that I would never remember correctly which one of them uses space or underscores as delimiter. It is also unexpected that both functions handle the remaining letters of the words differently (converting to lowercase or leaving as it is).

Though, I have no proposal for a better naming.

@wborn
Copy link
Member

wborn commented Oct 6, 2023

What's your position wrt this PR?

I'm OK with merging it if it's just a couple of utility methods.
Maybe the functionality also ends up in some newer Java version one day. 🙂

@J-N-K
Copy link
Member

J-N-K commented Oct 6, 2023

I'm fine with that.

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

lsiepel commented Oct 13, 2023

Thanks for implementing all the review comments!

My last point is about this: I have the feeling that calling two methods capitalizeWords() and capitalizAllWords() is not right way. I assume that I would never remember correctly which one of them uses space or underscores as delimiter. It is also unexpected that both functions handle the remaining letters of the words differently (converting to lowercase or leaving as it is).

Though, I have no proposal for a better naming.

The method nameshave been improved. Let me know if they need to be further adjusted or this can be merged.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks, finally some StringUtils at our disposal again. 😄

@kaikreuzer kaikreuzer merged commit 4001161 into openhab:main Oct 15, 2023
3 checks passed
@kaikreuzer kaikreuzer added the enhancement An enhancement or new feature of the Core label Oct 15, 2023
@kaikreuzer kaikreuzer added this to the 4.1 milestone Oct 15, 2023
@lsiepel lsiepel deleted the core-utils branch October 15, 2023 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add string utils in core
6 participants