-
-
Notifications
You must be signed in to change notification settings - Fork 429
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
[ephemeris] Added Ephemeris service #506
Conversation
Correction: ...to the compile-bom, runtime-bom AND the bundle pom itself? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some minor comments.
I also reviewed (a little bit) the code regardless if it has "only" been ported...
<artifactId>org.openhab.core.config.core</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not add any external dependency here.
That should be injected by the compile BOM already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I expected as well. But as you can see here, the PR build failed without it and once I added it, it succeeded. Any idea why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine it is working (building the project after removing that lines).
Looking at the check of the first commit:
- continuous-integration/travis-ci/pr: success
- PR-openHAB-Core: failure
I assume you should check your Jenkins configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO my comment is still valid and should be addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely - sorry, it slipped through. Fixed now.
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
@maggu2810 Any idea why travis failed? Did I do anything wrong wrt capability/requirement? |
You define a capability but no option how it could be resolved it is it not already be resolved by the current environment. See: openhab-core/features/karaf/openhab-core/src/main/feature/feature.xml Lines 23 to 33 in 89a54e5
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/caldav-binding-to-read-holidays-calendar/66367/13 |
Thanks - hopefully it is fixed now. |
bundles/org.openhab.core.ephemeris/.settings/org.eclipse.jdt.core.prefs
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...pt/src/org/eclipse/smarthome/model/script/internal/engine/action/EphemerisActionService.java
Outdated
Show resolved
Hide resolved
<artifactId>org.openhab.core.config.core</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On my machine it is working (building the project after removing that lines).
Looking at the check of the first commit:
- continuous-integration/travis-ci/pr: success
- PR-openHAB-Core: failure
I assume you should check your Jenkins configuration.
@kaikreuzer I wrote the Jollyday library. If there is anything I can do to help please contact me. |
Is there an ETA on a beta version of the Ephemeris service? I'm happy to be an early tester. |
@kaikreuzer Do you plan to update this PR? |
I had put in on hold as long as I didn't have a proper IDE. Will try to find time to revisit it. @clinique As this code was written by you, feel free to help getting it merged by doing PRs against this branch :-) |
Also-by: Gaël L'hopital <[email protected]> Signed-off-by: Kai Kreuzer <[email protected]>
Signed-off-by: Kai Kreuzer <[email protected]>
Signed-off-by: Kai Kreuzer <[email protected]>
Signed-off-by: Kai Kreuzer <[email protected]>
All, sorry for the delay. All comments should be addressed now. |
<artifactId>org.openhab.core.config.core</artifactId> | ||
<version>${project.version}</version> | ||
</dependency> | ||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO my comment is still valid and should be addressed.
...nhab.core.ephemeris/src/main/java/org/eclipse/smarthome/core/ephemeris/EphemerisManager.java
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...emeris/src/main/java/org/eclipse/smarthome/core/ephemeris/internal/EphemerisManagerImpl.java
Outdated
Show resolved
Hide resolved
...pt/src/org/eclipse/smarthome/model/script/internal/engine/action/EphemerisActionService.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kai Kreuzer <[email protected]>
Signed-off-by: Kai Kreuzer <[email protected]>
Thanks, @maggu2810, I have addressed all your comments. |
.../org.openhab.core.model.script/src/org/eclipse/smarthome/model/script/actions/Ephemeris.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Kai Kreuzer <[email protected]>
Seems I still got something wrong wrt to the feature definitions... |
@@ -114,6 +114,16 @@ | |||
<bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.linuxsysfs/${project.version}</bundle> | |||
</feature> | |||
|
|||
<feature name="openhab-core-ephemeris" version="${project.version}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You miss the ephemeris bundle itself in this very specific feature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. No idea how that got lost...
Signed-off-by: Kai Kreuzer <[email protected]>
After building a local distro I now see the following exceptions:
|
I did not realize the namesoace on review. |
@kaikreuzer Can you comment? |
Because of #506 (comment): "Ported over from eclipse-archived/smarthome#6227". This was a contribution to ESH, which just wasn't merged on time. |
We renamed the whole automation part that has already been part of ESH. |
For automation it was exceptionally important as this code was a new API that many bindings immediately wanted to use, so a lot of code was implemented using it. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/jython-logaction-not-working/77115/7 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/preparation-for-2-5m2/75738/1 |
Also-by: Gaël L'hopital <[email protected]> Signed-off-by: Kai Kreuzer <[email protected]> GitOrigin-RevId: d0dfbe3
Ported over from eclipse-archived/smarthome#6227.
@maggu2810 Did I do it correctly to add the jollyday lib to the compile- and runtime-bom?
Also-by: Gaël L'hopital [email protected]
Signed-off-by: Kai Kreuzer [email protected]