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

Add python_script response #97937

Merged
merged 10 commits into from Jan 5, 2024

Conversation

rikroe
Copy link
Contributor

@rikroe rikroe commented Aug 6, 2023

Breaking change

Service calls to python_script no longer silently fail and will raise an exception instead of being logged. This will stop scripts or automations instead of ignoring the error. continue_on_error must be set for scripts and automations using python_script that may fail.

Proposed change

Adds the new ServiceResponse to python_script services to allow for cases where we don't need to persist data from a python script.

The changes in core.ServiceRegistry.register where required as python_script discovers the files and registers the services in a non-async function.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

@rikroe
Copy link
Contributor Author

rikroe commented Sep 24, 2023

Thanks for the hint. I have adjusted the code to comply with the service rules for the Silver integration quality: ValueError on invalid inputs (assuming an invalid script is also invalid input) and HomeAssistantError for everything else.

Copy link
Contributor

@RoboMagus RoboMagus left a comment

Choose a reason for hiding this comment

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

LGTM

@rikroe
Copy link
Contributor Author

rikroe commented Sep 27, 2023

Added a breaking changes paragraph as raising exceptions will stop scripts/automations.

@RoboMagus
Copy link
Contributor

Added a breaking changes paragraph as raising exceptions will stop scripts/automations.

Now that you mention it. That would best be handled as it always has. I.e. the exceptions should only be passed on when providing service responses (service.return_response).

@rikroe
Copy link
Contributor Author

rikroe commented Sep 27, 2023

That could be an option as well. I'll leave that for the core team to decide, as it would be easy to implement (but from my understanding the raising exceptions is the desired path, even when not returning a response).

This comment was marked as resolved.

@github-actions github-actions bot added the stale label Dec 8, 2023
homeassistant/core.py Outdated Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft December 8, 2023 10:11
@home-assistant
Copy link

home-assistant bot commented Dec 8, 2023

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Dec 27, 2023

I suggest we keep the existing behavior around logging and exceptions for the existing features in this PR and only raise when a service response is requested.

Then in another PR we can make a breaking change. It's good to limit breaking changes PRs to as little scope as possible.

@MartinHjelmare MartinHjelmare marked this pull request as draft December 27, 2023 20:28
@rikroe
Copy link
Contributor Author

rikroe commented Dec 29, 2023

I have added some additional code to keep the existing logging possible if no response is required.

I am now also creating an issue to point users to the new behavior. Is 3 releases a good timeframe for this?

@rikroe rikroe marked this pull request as ready for review December 29, 2023 12:00
@home-assistant home-assistant bot marked this pull request as draft December 30, 2023 00:29
@rikroe rikroe marked this pull request as ready for review December 30, 2023 15:55
@emontnemery emontnemery dismissed MartinHjelmare’s stale review January 5, 2024 13:29

The issue has been removed

Copy link
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @RoboMagus 👍

@emontnemery emontnemery merged commit e7573c3 into home-assistant:dev Jan 5, 2024
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants