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

[tuya] Improve decode color format error handling #541

Merged
merged 1 commit into from
Dec 24, 2023

Conversation

jsetton
Copy link
Contributor

@jsetton jsetton commented Dec 21, 2023

Fixes #540

@jsetton jsetton requested a review from J-N-K as a code owner December 21, 2023 06:16
@J-N-K
Copy link
Member

J-N-K commented Dec 23, 2023

I would suggest a different solution, that also covers illegal formats for other value: Surround l. 166 to l. 193 with

try {

...

} catch (IllegalArgumentException ignored) {
}
updateState(channelId, UndefType.UNDEF);

And keep the log we have now. WDYT?

@jsetton
Copy link
Contributor Author

jsetton commented Dec 23, 2023

What about handling Integer.parseInt exception? I also personally rather have a try statement in a utility function than clogging the handler function or at the very least only cover the statements that would generate the catch exception.

As far as setting the channel to UNDEF, I don't mind doing it at that the bottom for any channel that wasn't updated but I have some reservation about the existing log line. In the use case I covered, I don't think it should log a warning since I always get this issue when my device is restarted and there isn't much that can be done to fix this. However it should certainly log a warning if it's an unknown data point.

@J-N-K
Copy link
Member

J-N-K commented Dec 23, 2023

I agree, but it sure is garbage. We can't know if it is a single "initial value" or we received an invalid value later, so I guess a warning is fine.

We usually try to return definite values and not null, it makes life much easier if it becomes a little bit more complex than here. That's why the method throws an exception instead of returning null. If it was Kotlin and we could use a safe-call or elvis operator I would agree that returning null is preferred.

I agree that we should use (IllegalArgumentException | NumberFormatException ignored) to also catch that one.

@J-N-K J-N-K added the bug Something isn't working label Dec 23, 2023
@jsetton jsetton force-pushed the tuya-color-format-error branch from ad1212b to 63dea16 Compare December 23, 2023 23:38
@jsetton
Copy link
Contributor Author

jsetton commented Dec 23, 2023

I made the changes per your comments. I forgot that NumberFormatException extends from IllegalArgumentException. So it is not needed.

Copy link
Member

@J-N-K J-N-K 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.

@J-N-K J-N-K merged commit a616368 into smarthomej:4.0.x Dec 24, 2023
2 checks passed
@J-N-K J-N-K added this to the 4.0.3 milestone Dec 24, 2023
@jsetton jsetton deleted the tuya-color-format-error branch December 24, 2023 16:30
J-N-K pushed a commit that referenced this pull request Dec 26, 2023
Signed-off-by: jsetton <[email protected]>
(cherry picked from commit a616368)
J-N-K pushed a commit that referenced this pull request Dec 26, 2023
Signed-off-by: jsetton <[email protected]>
(cherry picked from commit a616368)
(cherry picked from commit 068e9f3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tuya] IllegalArgumentException: Unknown color format
2 participants