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 data to service extensions for services that lacks metadata #1093

Merged
merged 7 commits into from
May 8, 2024

Conversation

helto4real
Copy link
Collaborator

@helto4real helto4real commented May 7, 2024

Breaking change

Proposed change

By adding the possibility to add custom data for services generated as extension methods on entities we can support service calls for services that does not provide metadata.

Example:
Calling a script can be called with custom data but since it lacks metadata the codegenerator generates empty calls.

Before:

public static class ScriptEntityExtensionMethods
{
...

    ///<summary>Runs the sequence of actions defined in a script.</summary>
    public static void TurnOn(this IScriptEntityCore target)
    {
        target.CallService("turn_on");
    }
...
}

After:

public static class ScriptEntityExtensionMethods
{
...

    ///<summary>Runs the sequence of actions defined in a script.</summary>
    public static void TurnOn(this IScriptEntityCore target, object? data = null)
    {
        target.CallService("turn_on", data);
    }
...
}

Also the same changes to servicecall. We add a data to service calls with no metadata but keeps the one with metadata the same:

Example of new generated optional data provided.

public partial class ScriptServices
{
    private readonly IHaContext _haContext;
    public ScriptServices(IHaContext haContext)
    {
        _haContext = haContext;
    }

    ///<summary>Reloads all the available scripts.</summary>
    public void Reload(object? data = null)
    {
        _haContext.CallService("script", "reload", null, data);
    }

    public void Setnightmode(object? data = null)
    {
        _haContext.CallService("script", "setnightmode", null, data);
    }

    ///<summary>Toggle a script. Starts it, if isn&apos;t running, stops it otherwise.</summary>
    ///<param name="target">The target for this service call</param>
    public void Toggle(ServiceTarget target, object? data = null)
    {
        _haContext.CallService("script", "toggle", target, data);
    }

    ///<summary>Stops a running script.</summary>
    ///<param name="target">The target for this service call</param>
    public void TurnOff(ServiceTarget target, object? data = null)
    {
        _haContext.CallService("script", "turn_off", target, data);
    }

    ///<summary>Runs the sequence of actions defined in a script.</summary>
    ///<param name="target">The target for this service call</param>
    public void TurnOn(ServiceTarget target, object? data = null)
    {
        _haContext.CallService("script", "turn_on", target, data);
    }
}

The code should not be breaking because of the optional data parameter.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)c
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

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][dev-checklist]
  • The code compiles without warnings (code quality check)
  • Tests have been added to verify that the new code works.

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

Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77%. Comparing base (dff49c8) to head (efd030a).

Additional details and impacted files
@@         Coverage Diff          @@
##           main   #1093   +/-   ##
====================================
  Coverage    77%     77%           
====================================
  Files       186     186           
  Lines      5168    5168           
  Branches    660     660           
====================================
+ Hits       3990    3993    +3     
- Misses     1030    1032    +2     
+ Partials    148     143    -5     
Flag Coverage Δ
unittests 77% <100%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@helto4real helto4real marked this pull request as draft May 7, 2024 12:48
@FrankBakkerNl
Copy link
Contributor

Just after approving I thought that it would need some basic tests

Copy link
Contributor

@FrankBakkerNl FrankBakkerNl left a comment

Choose a reason for hiding this comment

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

Looks easy enough, just needs one or two tests to check the code compiles with and without pasing an argument here

@helto4real
Copy link
Collaborator Author

Looks easy enough, just needs one or two tests to check the code compiles with and without pasing an argument here

I agree I will do tests :)

@@ -97,6 +97,8 @@ private static IEnumerable<MemberDeclarationSyntax> GenerateServiceMethod(string

if (serviceArguments is null)
{
targetParam = "object? data";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think traget is the ServiceTarget, eg the Entitiy, Area, Floor etc. This can be present or not regardless of how many servcieArguments there are. So you need to join them like is done at the third yield. (maybe reorganize so the code does not need to be duplicated)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the case when there are no service arguments, today it does not generate any input. Not sure I follow. I will check it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

 // method using arguments as separate parameters
            yield return ParseMemberDeclaration($$"""
                        void {{serviceMethodName}}({{JoinList(targetParam, serviceArguments.GetParametersList())}})

that third yield is when there are service targets. This is the not the case here. I only want to add the data option if there are no targets.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is the correct case indeed, just that you now override the serviceTarget with the data parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added tests and the case you were talking about in Discord. Actually found the problem when I did the test 👍

@helto4real helto4real marked this pull request as ready for review May 7, 2024 16:03
@helto4real helto4real merged commit 0d4392d into main May 8, 2024
7 checks passed
@helto4real helto4real deleted the add-service-call-data branch May 8, 2024 05:42
@helto4real
Copy link
Collaborator Author

Merged after Frank gave ok on discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants