Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Issue #601 : Introduce an optional unit in Number item, now that CQ h… #1857

Closed
wants to merge 75 commits into from

Conversation

clinique
Copy link
Contributor

…as been approved.

Not sure I included the libraries in the right way (I suspect it should be pushed to the target platform definition), but well here it is at last.

Signed-off-by: Gaël L'hopital glhopital@gmail.com

@maggu2810
Copy link
Contributor

Not sure I included the libraries in the right way

We have a CQ for

  • unit-api Version: 0.8
  • unit-ri Version: 0.8

You are using version 0.9 know.
Have there other CQs created that are not linked in #601?

Can you point me to the CQ for uom-lib-common-0.9?

@maggu2810
Copy link
Contributor

After #1858 and #1859 are applied, you do not need to bundle the JARs (at least the two I seen the CQs for) anymore.

@kaikreuzer
Copy link
Contributor

Applied ;-)

@clinique
Copy link
Contributor Author

Sorry, but I do not get where are the merge conflicts...

@clinique
Copy link
Contributor Author

clinique commented Jul 13, 2016

I guess that once this is included, we'll have to impact it's capabilities in channel-type definitions.
If I take the example of (coming from yahooweather) :

<channel-type id="temperature">
    <item-type>Number</item-type>
    <label>Temperature</label>
    <description>Current temperature in degrees celsius</description>
    <category>Temperature</category>
    <state readOnly="true" pattern="%.1f °C">
    </state>
</channel-type>

should evolve to something like :

<channel-type id="temperature">
    <item-type>Quantity</item-type>
    <label>Temperature</label>
    <description>Current temperature</description>
    <category>Temperature</category>
    <state readOnly="true" pattern="%s">
    </state>
</channel-type>

so dimension of the item should be defined by the category and default representation of a Temperature be setup at system level.
I imagine that we then don't mind wether the binding gets the data in °C or °F, the representation will be adapted to end-user preference ?
But maybe it's not the place to discuss impacts of this PR on the whole system...?

@maggu2810
Copy link
Contributor

Sorry, but I do not get where are the merge conflicts...

I assume the file targetplatform/smarthome.target should not be part of your PR.

@clinique
Copy link
Contributor Author

Ok, removing it.

@maggu2810
Copy link
Contributor

Ok, removing it.

Did not work, it is still part of the PR.

@clinique
Copy link
Contributor Author

No, seems fine, no ?

@maggu2810
Copy link
Contributor

Yes, seems fine, now. My browser missed perhaps a refresh.

@clinique
Copy link
Contributor Author

@kaikreuzer : so, are we clear to merge this ?

@kaikreuzer
Copy link
Contributor

Unfortunately not.
I just finished a discussion with the IP team - the unit-ri library isn't approved after a detailed analysis (it contains GPL/JDK code) and thus we will have to remove it from ESH again.

The new plan is now to use https://github.com/unitsofmeasurement/uom-se, which is more or less the same, but builds on Java8 (and thus does not need to include any JDK code as it can use it from the JDK8 itself). For this PR, this should not change much as the API stays the same.
Nonetheless, we have to wait for two things:

  1. Have CQ12332 (for uom-se 1.0.2) and CQ12206 (for unit-api 1.0) approved
  2. Have ESH moved to Java8 as a minimal requirement

I hope that both will happen within a few weeks from now, rather than months.
Sorry for having to ask for your patience again!

@kaikreuzer kaikreuzer added the CQ label Nov 30, 2016
@clinique
Copy link
Contributor Author

Ok, so when these CQ are approved and Java8 movement validated, I'll review the PR.

@maggu2810
Copy link
Contributor

After the c3 of the resp. CQ the bundle has been added to the third-party p2 repo of ESH:
https://github.com/eclipse/smarthome/blob/gh-pages/third-party/pom.xml#L69

We should remove it now again...

@maggu2810
Copy link
Contributor

... I should read to the end of the thread.

so I will make sure to remove unit-ri 0.8 from the Eclipse SmartHome repository in due time again.

Haven't realized that you already plan to do so.

@kaikreuzer
Copy link
Contributor

Good news, CQ 12332 for unit-se has already been approved for check-in. Now only waiting for unit-api.

@clinique
Copy link
Contributor Author

...in the starting blocks :)

@watou watou mentioned this pull request Dec 29, 2016
@kaikreuzer
Copy link
Contributor

Good news - unit-api 1.0 has also been approved for check-in.
I have just updated the smarthome.target to use it, see #2722.
Note that for the moment, the implementation (uom-se) is still missing on the target platform, so while you can compile your code against the API, you cannot test it. I'll look for a way to make uom-se also available.

@kaikreuzer
Copy link
Contributor

FTR: unitsofmeasurement/uom-se#134

@watou
Copy link
Contributor

watou commented Dec 30, 2016

@kaikreuzer
Copy link
Contributor

Yes, please see above

@watou
Copy link
Contributor

watou commented Dec 30, 2016

Ah, thanks.

@kaikreuzer
Copy link
Contributor

FTR: I have created CQ 12518 for uom-lib-common, which is a required dependency of uom-se.

@hakan42
Copy link
Contributor

hakan42 commented Jan 13, 2017

@kaikreuzer from your experience, do you expect this issue to be resolved in time for OH2 release? Or, would you wait with OH2 "final" for this to land?

@clinique
Copy link
Contributor Author

My 2 cents is that including this and creation of the QuantityItem is only the begining of a whole bunch of modifications in ESH to take it in account fully. So I would not expect it fully deployed very quick...

@kaikreuzer
Copy link
Contributor

I share @clinique's cents - this is going to be a major new feature, which will probably also require many bindings to be changed in order to support it, so I am not planning it for openHAB 2.0.0 (feature freeze tomorrow!), but rather some 2.x.

@kaikreuzer
Copy link
Contributor

FTR, I have created CQ 12610 for uom-se 1.0.3, which we need for the target platform.

Henning Treu added 12 commits December 8, 2017 10:52
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu
Copy link
Contributor

htreu commented Dec 8, 2017

Hi @clinique please see https://github.com/clinique/smarthome/pull/6 for yet another final attempt to get this going ;-) Please review and merge.
I hope merging will be painless since I already rebased it on this PR. Rebasing onto master might be another issue.

again, thank you very much!

clinique and others added 11 commits December 11, 2017 14:08
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu
Copy link
Contributor

htreu commented Dec 22, 2017

Hi @clinique, yet another PR to be merged. See https://github.com/clinique/smarthome/pull/7.
@kaikreuzer and me did schedule a review session for next week so this will hopefully come to an end soon(TM) ;-)

Issue601 - latest cleanup
@clinique
Copy link
Contributor Author

@htreu : you made an amazing work! Thanks a lot for pushing this, it was far beyond my knowledge to do it.

@kaikreuzer
Copy link
Contributor

Superseded by #4818 (as I didn't have push rights to your branch @clinique to resolve the conflicts).

@kaikreuzer kaikreuzer closed this Dec 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants