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.

Sign up for GitHub

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

Jump to bottom

[automation] Added 'EphemerisConditions' for NGRE #915

Merged
merged 6 commits into from
Oct 19, 2019
Merged

[automation] Added 'EphemerisConditions' for NGRE #915

merged 6 commits into from
Oct 19, 2019

Conversation

cweitkamp
Copy link
Contributor

@cweitkamp cweitkamp commented Jul 10, 2019 •
edited
Loading

  • Added EphemerisConditions for NGRE

Should this be placed in an own package org.openhab.core.automation.module.ephemeris?

Signed-off-by: Christoph Weitkamp [email protected]

5iver
5iver suggested changes Jul 10, 2019
View reviewed changes
Copy link

@5iver 5iver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of your automation PRs were merged before I could get a test done. This time I'm going to review first and then test. 😄 Although, I'm not sure that is going to be possible due to #903.

Should this be placed in an own package org.openhab.core.automation.module.ephmeris?

IMO, there's no need to separate it. I also question why you didn't put the factory into CoreModuleHandlerFactory with all the other core module handler factories.

...ab.core.automation/src/main/resources/ESH-INF/automation/moduletypes/EphmerisConditions.json Outdated Show resolved Hide resolved
...ab.core.automation/src/main/resources/ESH-INF/automation/moduletypes/EphmerisConditions.json Outdated Show resolved Hide resolved
...c/main/java/org/openhab/core/automation/internal/module/handler/CompareConditionHandler.java Show resolved Hide resolved
@cweitkamp
Copy link
Contributor Author

I also question why you didn't put the factory into CoreModuleHandlerFactory with all the other core module handler factories.

Because - to be honest - for practice reasons. And I was not sure if I should split the packages. If you think the packaging is okay, I will merge them and change the Condition UID to something like "ephmeris.HolidayCondition" and "ephmeris.WeekendCondition".

@5iver
Copy link

5iver commented Jul 10, 2019 •
edited
Loading

Yeah... I don't see any benefit in splitting it out. It's nice to see them all in one place too.

Just thought of something... it should be really easy to add a weekday condition too.

@cweitkamp cweitkamp force-pushed the feature-ngre-ephemeris-conditions branch from 0f7cbdf to c577b4e Compare July 11, 2019 09:00
@cweitkamp
Copy link
Contributor Author

The only reason to keep the factory is the reference to the EphmerisManager.

5iver
5iver suggested changes Jul 11, 2019
View reviewed changes
Copy link

@5iver 5iver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One typo and a couple suggestions

...ab.core.automation/src/main/resources/ESH-INF/automation/moduletypes/EphmerisConditions.json Outdated
"name": "dayset",
"type": "text",
"label": "Dayset",
"description": "Name of the requested dayset, witout prefix.",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

witout -> without

...ab.core.automation/src/main/resources/ESH-INF/automation/moduletypes/EphmerisConditions.json Outdated
},
{
"uid": "ephmeris.WeekdayCondition",
"label": "It is a day in a week",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a weekday

...ab.core.automation/src/main/resources/ESH-INF/automation/moduletypes/EphmerisConditions.json Outdated
},
{
"uid": "ephmeris.WeekendCondition",
"label": "It is a day on the weekend",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a weekend

5iver
5iver approved these changes Jul 11, 2019
View reviewed changes
Copy link

@5iver 5iver left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

maggu2810
maggu2810 reviewed Jul 11, 2019
View reviewed changes
.../java/org/openhab/core/automation/internal/module/factory/EphemerisModuleHandlerFactory.java Outdated
}
}

logger.error("The ModuleHandler is not supported:{}", moduleTypeUID);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be a whitespace after the colon?

@cweitkamp cweitkamp changed the title [automation] Added EphmerisConditions for NGRE [automation] Added EphemerisConditions for NGRE Jul 25, 2019
@cweitkamp cweitkamp force-pushed the feature-ngre-ephemeris-conditions branch 2 times, most recently from e1946e1 to 9d2aabb Compare July 26, 2019 07:54
@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/ephemeris-binding/64102/10

@cweitkamp cweitkamp added awaiting other PR Depends on another PR and removed awaiting other PR Depends on another PR labels Oct 2, 2019
@cweitkamp cweitkamp force-pushed the feature-ngre-ephemeris-conditions branch from 9d2aabb to 3d4e735 Compare October 5, 2019 19:34
@cweitkamp cweitkamp requested review from 5iver and a team October 5, 2019 19:35
@cweitkamp
Fixed warnings and formatted JSON files
98a8c3b 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp
Added 'EphmerisConditions' for NGRE
7247447 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp
Fixed spelling; Incorporated comments from review
6590d4d 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp
Incorporated changes from review
7362864 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp
Fixed spelling
454d6a0 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp
Resolved merge conflicts
4a6d24b 
Signed-off-by: Christoph Weitkamp <[email protected]>
@cweitkamp cweitkamp force-pushed the feature-ngre-ephemeris-conditions branch from 3d4e735 to 4a6d24b Compare October 8, 2019 08:13
kaikreuzer
kaikreuzer approved these changes Oct 19, 2019
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks useful, thanks!

@kaikreuzer kaikreuzer merged commit a3d91be into openhab:master Oct 19, 2019
@cweitkamp cweitkamp deleted the feature-ngre-ephemeris-conditions branch October 20, 2019 07:31
@cweitkamp cweitkamp added this to the 2.5 milestone Oct 20, 2019
@cweitkamp cweitkamp added the enhancement An enhancement or new feature of the Core label Oct 20, 2019
@cweitkamp cweitkamp changed the title [automation] Added EphemerisConditions for NGRE [automation] Added 'EphemerisConditions' for NGRE Dec 15, 2019
Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020
@jmontleon @Confectrician
Fixes openhab#911 - Improve Windows Service Logging (openhab#915)
1f6703d
splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
@cweitkamp @splatch
[automation] Added EphemerisConditions for NGRE (openhab#915)
0b22d4a 
Signed-off-by: Christoph Weitkamp <[email protected]>
GitOrigin-RevId: a3d91be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maggu2810 maggu2810 maggu2810 left review comments

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@5iver 5iver Awaiting requested review from 5iver

Assignees
No one assigned
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
2.5
Development

Successfully merging this pull request may close these issues.
6 participants
@cweitkamp @5iver @openhab-bot @maggu2810 @kaikreuzer @wborn