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

[core] Added standard gravity and dBm Units #617

Merged
merged 2 commits into from
Mar 1, 2019

Conversation

Hilbrand
Copy link
Member

@Hilbrand Hilbrand commented Mar 1, 2019

Also did some clean up, which is in the first commit. The units are in the second commit.
Related to: https://github.com/openhab/openhab2-addons/pull/4913

Hilbrand added 2 commits March 1, 2019 12:53
The classes ImperialUnits and SIUnits both extended SmartHomeUnits. But the classes are used a singletons.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Signed-off-by: Hilbrand Bouwkamp <[email protected]>
@maggu2810
Copy link
Contributor

What's the reason to make the classes final and its constructor private?
Do you see any critical if someone wants to create a sub class so we need to prevent it that strong?
Sure, it would make no sense (perhaps) but does it harm if?

@maggu2810
Copy link
Contributor

I assume it has been wrong before but has been possible to use every "SmartHomeUnit" as "SIUnit" and "ImperialUnit".

        System.out.println(SmartHomeUnits.AMPERE);
        System.out.println(SIUnits.AMPERE);
        System.out.println(ImperialUnits.AMPERE);

It is not valid anymore.

It seems to be correct after the change because a SmartHomeUnit is not necessary an SIUnit.

But should we mark this as "API breaking" (and merge it) as it can break the consumers.

@Hilbrand
Copy link
Member Author

Hilbrand commented Mar 1, 2019

The classes already had private constructors, except for SmartHomeUnits that had a protected constructor. Because of the private constructors is seems logical to make this more explicit and make the classes final. SmartHomeUnits was kind of misused by the other to classes. They extended SmartHomeUnits, but created a singleton of their own class, even while SmartHomeUnits also is used as a singleton class. So I changed the extend to the AbstractSystemOfUnits class they should extend (I think) and with that change SmartHomeUnits can have the private constructor too (and be a final class).

@Hilbrand
Copy link
Member Author

Hilbrand commented Mar 1, 2019

I checked the openhab2 repo and found 2 issues of problems due to the changes, but only in tests. I've created a pr (5016) to fix them.

@maggu2810
Copy link
Contributor

Thanks

@maggu2810 maggu2810 merged commit 4517f39 into openhab:master Mar 1, 2019
@Hilbrand Hilbrand deleted the units branch March 2, 2019 11:17
Hilbrand added a commit to Hilbrand/openhab-core that referenced this pull request Mar 2, 2019
The change with unit classes parent (openhab#617) causes it to report missing imports of tec.uom.se when using static import. Before the change this was not a problem because the parent class was SmartHomeUnits. This change adds a new parent class for all units.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
Hilbrand added a commit to Hilbrand/openhab-core that referenced this pull request Mar 2, 2019
The change with unit classes parent (openhab#617) causes it to report missing imports of tec.uom.se when using static import. Before the change this was not a problem because the parent class was SmartHomeUnits. This change adds a new parent class for all units.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
wborn pushed a commit that referenced this pull request Mar 2, 2019
The change with unit classes parent (#617) causes it to report missing imports of tec.uom.se when using static import. Before the change this was not a problem because the parent class was SmartHomeUnits. This change adds a new parent class for all units.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
@wborn wborn added the UoM Units of Measurement label Mar 5, 2019
@wborn wborn added this to the 2.5 milestone Mar 5, 2019
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/started-getting-compile-error-after-mvn-clean/69799/3

@cweitkamp cweitkamp changed the title Added standard gravity and dBm Units [core] Added standard gravity and dBm Units Dec 7, 2019
@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 7, 2019
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
The change with unit classes parent (openhab#617) causes it to report missing imports of tec.uom.se when using static import. Before the change this was not a problem because the parent class was SmartHomeUnits. This change adds a new parent class for all units.

Signed-off-by: Hilbrand Bouwkamp <[email protected]>
GitOrigin-RevId: caa9b71
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core UoM Units of Measurement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants