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

[fronius] Thing actions: Return boolean & Annotate all inputs as required #17623

Merged
merged 3 commits into from
Oct 29, 2024

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Oct 23, 2024

  • Returns a boolean from all Thing actions indicating the action was executed successful or not.
  • Annotates all action inputs as required.

…ailure

Signed-off-by: Florian Hotze <dev@florianhotze.com>
@florian-h05 florian-h05 requested a review from trokohl as a code owner October 23, 2024 21:38
Signed-off-by: Florian Hotze <dev@florianhotze.com>
@florian-h05 florian-h05 changed the title [fronius] Thing actions: Return boolean to indicate success/failure [fronius] Thing actions: Return boolean & Annotate all inputs as required Oct 23, 2024
@florian-h05 florian-h05 requested a review from jlaur October 23, 2024 21:48
@lolodomo
Copy link
Contributor

For information, look at #17504.
I know you want to remove @ActionOutput but it has to be discussed first.

@florian-h05
Copy link
Contributor Author

Given the core code and the current docs, actions that return a single value don’t need @ActionOutput. This annotation is not read by core, it is currently totally useless.
So I’m not in favour adding it here, whereas my changes here improve the experience.

@florian-h05
Copy link
Contributor Author

florian-h05 commented Oct 24, 2024

So IMHO this PR can be merged and @ActionOutput can still be added later.

This PR is unrelated to @ActionOutput discussion.

@lolodomo
Copy link
Contributor

lolodomo commented Oct 27, 2024

This PR is unrelated to @ActionOutput discussion.

It is in the sense you are adding output to thing actions in this PR.
We must keep consistency between the way to do in all bindings.
So let's decide first what we do globally with these actions annotations and then we can merge this PR.
Let's conclude the discussion in the issue.

@florian-h05 florian-h05 marked this pull request as draft October 27, 2024 14:23
@lolodomo
Copy link
Contributor

@florian-h05 : I let you adjust as finally decided.

As discussed in openhab#17504.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
@florian-h05 florian-h05 force-pushed the fronius-actions-boolean branch from f69ad0f to b075b79 Compare October 29, 2024 20:50
@florian-h05 florian-h05 marked this pull request as ready for review October 29, 2024 20:55
@florian-h05
Copy link
Contributor Author

@lolodomo I have just added the @ActionOutput annotations.

}

public void addHoldBatteryChargeSchedule(ZonedDateTime from, ZonedDateTime until) {
addHoldBatteryChargeSchedule(from.toLocalTime(), until.toLocalTime());
public boolean addHoldBatteryChargeSchedule(ZonedDateTime from, ZonedDateTime until) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Annotations are missing for this action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is deliberately, I don’t need those overrides to show up in the UI or be available as rule module. These overrides solely exist to allow easier usage from scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is unusual but ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure which binding it was but not annotation any existing override helps improving the UI experience.

}

public void addForcedBatteryChargingSchedule(ZonedDateTime from, ZonedDateTime until, QuantityType<Power> power) {
addForcedBatteryChargingSchedule(from.toLocalTime(), until.toLocalTime(), power);
public boolean addForcedBatteryChargingSchedule(ZonedDateTime from, ZonedDateTime until,
Copy link
Contributor

Choose a reason for hiding this comment

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

Annotations are missing for this action.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@lolodomo lolodomo merged commit 7f256a7 into openhab:main Oct 29, 2024
5 checks passed
@lolodomo lolodomo added this to the 4.3 milestone Oct 29, 2024
@florian-h05 florian-h05 deleted the fronius-actions-boolean branch October 29, 2024 21:35
KaaNee pushed a commit to KaaNee/openhab-addons that referenced this pull request Nov 8, 2024
…ired (openhab#17623)

* [fronius] Symo Inverter actions: Return boolean to indicate success/failure
* [fronius] Symo Inverter actions: Annotate all inputs as required
* [fronius] Add `@ActionOutput` annotation

As discussed in openhab#17504.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
matchews pushed a commit to matchews/openhab-addons that referenced this pull request Dec 16, 2024
…ired (openhab#17623)

* [fronius] Symo Inverter actions: Return boolean to indicate success/failure
* [fronius] Symo Inverter actions: Annotate all inputs as required
* [fronius] Add `@ActionOutput` annotation

As discussed in openhab#17504.

Signed-off-by: Florian Hotze <dev@florianhotze.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants