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

Legrand with Netatmo cluster 0xfc01 support #736

Merged
merged 1 commit into from
Feb 5, 2022

Conversation

pgaufillet
Copy link
Contributor

Implement Legrand with Netatmo cluster 0xfc01 support as described in issue #733.

Signed-off-by: Pierre Gaufillet [email protected]

Copy link
Contributor

@cdjackson cdjackson 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 this. It generally looks fine to me - just a couple of small comments/questions/changes.

* @param s String that shall be unescaped.
* @return UTF16 string
*/
private static final String unescape(String s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be static? I can't immediately see a need for that so if it's not required then I'd suggest to remove this.

Copy link
Contributor Author

@pgaufillet pgaufillet Feb 3, 2022

Choose a reason for hiding this comment

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

unescape is rather a utility method than a member of TigBeeThingTypeMatcher, as it does not apply to its properties but to a String argument. In this kind of case, I usually create a Util class for holding such methods as static ones. Here, with only one util method, keeping it in the user class looked to be the best choice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, but I see it as just another method in the class and I think in general making all (or really, any) methods static is not highly desirable. If this was a helper class where you were providing static methods externally, then I could see some merit, but as it is it's all internal and private.

I'm happy for it to be in the user class - it certainly makes no sense for it to be elsewhere - I just don't see the need to make it static.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Removed "final" too after having thought about it.

@@ -172,4 +172,30 @@ public RequiredProperty(String name, String value) {
this.value = value;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this needs formatting as this method doesn't line up with the method above. It looks like tabs have been used rather than spaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I accidentally applied spotless settings to all the code and had to cancel it later. This is probably a resulting error. I am going to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied openhab formatter style to this method. It should be ok now. Same thing for the import order.

Handles invisible/special characters with \u<xxxx> Unicode escape
notation.

Signed-off-by: Pierre Gaufillet <[email protected]>
@cdjackson
Copy link
Contributor

Thanks for this @pgaufillet - looks good now.

@cdjackson cdjackson merged commit 7f0ad82 into openhab:main Feb 5, 2022
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.

2 participants