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

[kaleidescape] Remove Apache StringEscapeUtils #14513

Merged
merged 2 commits into from
Mar 26, 2023

Conversation

mlobstein
Copy link
Contributor

Replace StringEscapeUtils.unescapeHtml4() with basic Java methods to address #7722. Also a couple very minor tweaks that I have had pending for some time now.

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
Copy link
Contributor

@lsiepel lsiepel left a comment

Choose a reason for hiding this comment

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

The code looks good. Only thing is, i can't tell if it handles all required cases. StringEscapeUtils.unescapeHtml4 handles ~290 different cases.
It would be really helpfull if a unit test was provided. Throwing in some examples into the stringformatter method.

@J-N-K
Copy link
Member

J-N-K commented Feb 27, 2023

IIRC we use unbescape in other add-ons

@lsiepel
Copy link
Contributor

lsiepel commented Feb 27, 2023

IIRC we use unbescape in other add-ons

And we try to get rid of them. Would be nice to have such a utility function in core. Please see #7722 (comment)

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
@J-N-K
Copy link
Member

J-N-K commented Feb 28, 2023

If you scroll up in that issue, you can see that this es exactly the reason why we introduced unbescape. It‘s hard to get it right.

@lsiepel
Copy link
Contributor

lsiepel commented Feb 28, 2023

hat issue, you can see that this es exactly the

Totally agree, unbescape got somewhat burried in that topic. i re-read and allready got a PR for BSBLAN with that dependency. Would be nice if someone could have commented in that topic about my30+ PR's though.

Regarding this PR, if all works as expected i would opt to merge it as is without any additional dependency, but it's not up to me.

@lsiepel lsiepel requested a review from a team February 28, 2023 09:57
@mlobstein
Copy link
Contributor Author

I added some tests. This implementation only needs to un-escape the Latin-1 characters as described in the protocol reference manual:
image

Copy link
Contributor

@lsiepel lsiepel 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
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!

@jlaur jlaur merged commit ec329c4 into openhab:main Mar 26, 2023
@jlaur jlaur added this to the 4.0 milestone Mar 26, 2023
@mlobstein mlobstein deleted the k_remove_apache branch March 26, 2023 13:44
FordPrfkt pushed a commit to FordPrfkt/openhab-addons that referenced this pull request Apr 20, 2023
* Remove Apache StringEscapeUtils
* add tests

Signed-off-by: Michael Lobstein <michael.lobstein@gmail.com>
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