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

[jsscripting] Fix JS Quantity to Java QuantityType conversion #16106

Merged
merged 1 commit into from Dec 25, 2023

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Dec 24, 2023

Regression from openhab/openhab-js#312.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

@jlaur That should fix it. I’ve created it from GitHub web and are not able to test it, so would be great if you could compile and deploy it and test it.

@florian-h05 florian-h05 marked this pull request as draft December 24, 2023 14:16
@jlaur
Copy link
Contributor

jlaur commented Dec 25, 2023

@florian-h05 - thank you! I have now tested the fix, and it works perfectly.

@jlaur jlaur added the bug An unexpected problem or unintended behavior of an add-on label Dec 25, 2023
@florian-h05 florian-h05 marked this pull request as ready for review December 25, 2023 12:56
@florian-h05
Copy link
Contributor Author

Fine, then let's merge this.
This should also be backported to 4.1.x

@jlaur
Copy link
Contributor

jlaur commented Dec 25, 2023

Fine, then let's merge this.
This should also be backported to 4.1.x

Yes. Do you want to test this also while waiting for the 4.1.x branch to be created? I only did one basic test by successfully running a rule that was previously broken - and requiring one of the EDS thing actions having QuantityType in the method signature.

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

@florian-h05
Copy link
Contributor Author

Do you want to test this also while waiting for the 4.1.x branch to be created?

I don’t think that’s necessary.

However I wonder if we should provide a backward compatibility layer so that the conversion still works for older versions of the library. Though I would expect every user that has openhab-js installed manually to upgrade it to the latest version, and as the library had no breaking changes for nearly a year now, nothing should hold back from upgrading to the latest version. You only get plenty of fixes and some new features.
WDYT?

@jlaur
Copy link
Contributor

jlaur commented Dec 25, 2023

However I wonder if we should provide a backward compatibility layer so that the conversion still works for older versions of the library. Though I would expect every user that has openhab-js installed manually to upgrade it to the latest version, and as the library had no breaking changes for nearly a year now, nothing should hold back from upgrading to the latest version. You only get plenty of fixes and some new features.
WDYT?

I'm not sure it's relevant in 4.1 to older versions of openhab-js than the version bundled with 4.1. I don't know if it it's relevant or even possible to support newer versions of openhab-js in 4.0 (in case of manual upgrade of openhab-js without upgrading to 4.1)?

@florian-h05
Copy link
Contributor Author

I'm not sure it's relevant in 4.1 to older versions of openhab-js than the version bundled with 4.1.

Not really, there is no reason to not upgrade to the latest openhab-js when moving to openHAB 4.1.

I don't know if it it's relevant or even possible to support newer versions of openhab-js in 4.0

That’s a good question. openhab-js still works on 4.0, but the automatic type conversion for Quantity will not work for the latest version.
IMO there is also no reason to not upgrade from 4.0 to 4.1, but it would be possible to modify the type conversion to support old and new versions of openHAB-JS. This fix then could be backported to 4.0.x to make newer openhab-js versions compatible with openHAB 4.0.x, but whether there will be a new 4.0.x release is another question.

Personally, I’d vote no keep this PR as is, and require users to upgrade their manually installed openhab-js to the latest version when moving to openHAB 4.1. For 4.0 users that upgrade to the latest version of the library, I would say, that only the latest openHAB release provides full compatibility with the latest version of the library and they should please upgrade openHAB.

@jlaur jlaur merged commit 20a6eee into openhab:main Dec 25, 2023
3 checks passed
@jlaur jlaur added this to the 4.2 milestone Dec 25, 2023
@florian-h05 florian-h05 deleted the jsscripting-quantity branch December 25, 2023 14:11
jlaur pushed a commit that referenced this pull request Dec 25, 2023
Regression from openhab/openhab-js#312.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@jlaur jlaur added the patch A PR that has been cherry-picked to a patch release branch label Dec 25, 2023
@jlaur jlaur changed the title [jssscripting] Fix JS Quantity to Java QuantityType conversion [jsscripting] Fix JS Quantity to Java QuantityType conversion Mar 3, 2024
austvik pushed a commit to austvik/openhab-addons that referenced this pull request Mar 27, 2024
…ab#16106)

Regression from openhab/openhab-js#312.

Signed-off-by: Florian Hotze <florianh_dev@icloud.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
bug An unexpected problem or unintended behavior of an add-on patch A PR that has been cherry-picked to a patch release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with Quantity to QuantityType conversion after upgrade to 4.1
2 participants