Draft
Conversation
Some message types (e.g., `BaseMessage` and `AuthMessage`) used the full spelled-out word "Message" in their names, while others (like `ChanMsg` and `stateChangedMsg`) used "Msg". For consistency, rename the latter to be more consistent with the former: * `ChanMsg` → `ChanMessage` * `stateChangedMsg` → `stateChangedMessage`
Some users of this library will need to subscribe to messages and/or send and receive individual websocket messages. So make those interfaces public.
All messages have a "type" and "id", but not all messages have a "success" field. So remove the `Success` field from `BaseMessage`. Add a `BaseResultMessage`, which consists of a `BaseMessage` plus a `Success` field, to use in those cases when the message does have a "success" field.
Now that `BaseMessage` doesn't include a `Success` field, it is a building block that we can use within `stateChangedMessage`.
Since we're pretending that these messages have "success" fields, it is more appropriate to call them `ResultMessage`s. Soon, that assumption will be relaxed.
Not all messages coming from HA will be result messages, so don't try to parse them as such. Instead, only parse the `BaseMessage` part, and pass them to listeners as `Message` objects, which contain the `BaseMessage` part plus the entire raw message as JSON.
Now that nobody is using `ResultMessage`, we're free to make it more capable: * Add a new `ResultError` type, for holding errors that were included in result messages. * If a message contains an "error" field, parse it and store it in a new `BaseResultMessage.Error` field of type `ResultError`. * Change `ResultMessage` to have a `Result` field rather than a `Raw` field. Store the unparsed "result" field from a result message to `ResultMessage.Result`. * Add a method `Message.GetResult()`, which allows a `Message` to be unmarshaled as a `ResultMessage` and its result unmarshaled into a user-provided variable. If the message includes and error, return that error as a Go error. Soon this new functionality will be used to handle the results of HA API calls.
This agrees with `websocket.BaseMessage` and with the JSON field name.
Introduce a new type, `CallServiceMessage`, to contain the entire message required to invoke an HA service. This type embeds an instance of `BaseServiceRequest`. Remove the `ID` and `Type` fields from `BaseServiceRequest`, because they don't need to be set by the caller, but are rather managed within `App.Call()`. Move these fields to `BaseServiceRequest` by embedding a `websocket.BaseMessage` instance the the latter type.
Rename method `App.Call()` to `App.CallAndForget()`, because this is a "fire-and-forget" style of calling an API function that doesn't wait for a response. In a moment we'll add a new `Call()` method that waits for and returns the response from the server.
At the `Conn` level, we don't want to discard any messages entirely, because for all we know somebody might be interested in them. (In the future, there will indeed be "result" message listeners.) So instead, change `conn.Run()` to pass all messages through, but change the event listener callback to discard any messages that arrive that don't match the desired message type.
Add a new version of method `App.Call()` that not only invokes a Home Assistant API (like `CallAndForget()`), but also waits for the server to respond and unmarshals the result into a caller-provided variable. If the server returns an error, return that error as a `*websocket.ResultError`. This makes it easy to call APIs that return results, and also makes it easy for the caller to detect when the requested action failed.
mhagger
force-pushed
the
message-package
branch
from
bad392e to
e8eb789
Compare
Change the types of `serviceData` arguments from `...map[string]any`
to `...any`. This allows the caller to pass a struct in, as long as
the struct can be serialized to a JSON object. For example, for a
light, one might define
```
type LightServiceData struct {
Brightness int `json:"brightness,omitzero"`
// ColorTemp is the color temperature in mireds
// (micro-reciprocal-degrees-Kelvin).
ColorTemp int `json:"color_temp,omitzero"`
}
```
and then call `TurnOn()` like
```
_, err := app.Service.Light.TurnOn(
ctx, l.target, ServiceData{Brightness: newBrightness},
)
```
When calling a service, use `Call()` rather than `CallAndForget()` so that the result of the service call is collected. Return the result to the caller as type `any`. (If people go to the trouble of figuring out the structure of the methods' responses, these return types can be made more specific. This requires a `context.Context` argument to be added to the service call arguments. It can be used, for example, to limit the amount of time to wait for a response from the server.
Currently, message types are declared in multiple packages: `websocket`, `gomeassistant`, `types`, `internal/services`, and maybe somewhere else that I forgot. Partly this is historical, but partly it is to avoid import cycles. There are also inconsistencies about how messages are named and whether they embed `BaseMessage`. Let's move all of the message types to a new `message` package. This package won't have dependencies on any other packages, so it can be imported from anywhere. Start by moving the following types to the new package, and adjusting the users: * `BaseMessage` * `Message` * `RawMessage` * `BaseResultMessage` * `ResultError` * `ResultMessage`
Move type `websocket.SubEvent` to the `message` package, rename it, and embed a `BaseMessage`.
This type is not used elsewhere, and it doesn't conform to the standard message layout, so make it local to the function where it is used.
This is more consistent with the other message definitions, not to mention with the JSON field that it corresponds to.
This type is not used elsewhere, and it doesn't conform to the standard message layout, so make it local to the function where it is used.
This is more consistent with the other message definitions, not to mention with the JSON field that it corresponds to.
We should really be using a `ctx` to allow a timeout. But that's not implemented now, so for now, remove the `ctx` arguments to these two methods.
It's not doing anything for us that `message.Message` can't already do.
Add an `EventMessage` type to represent an event message, parameterized by the type of the event's "data" field. Also add supporting types `Event`, `EventOrigin`, and `EventContext` types to represent components of the message. The new types are not yet used.
Move type `StateChangedMessage`, along with its related types `StateData` and `MessageState`, from the top-level package to `message`, make them public, and tweak the name `msgState` → `MessageState`.
`StateChangedMessage` is also an `EventMessage`, just with a different data payload. So pull it into the system: * Rename `StateChangedMessage` to `StateChangedEventMessage`. * Change the definition of `StateChangedEventMessage` to `EventMessage[StateChangedData]`. This adds some fields that were omitted before. * Rename `StateData` to `StateChangedData`. * Rename `MessageState` to `StateChangedState` and add some fields that were omitted before.
Instead of extracting the entity data from an `EntityChangeEventMessage`, putting it in an `EntityData`, and passing the `EntityData` to the `EntityListener`s, just pass the `StateChangedData` structure to the listeners. This requires one less type, gives the listeners more information to work with, and makes the information that listeners get agree with the Home Assistant documentation about change events. This requires listeners' callbacks to have a different signature, but the change is pretty straightforward.
Move type `EventZWaveJSValueNotification` from the top-level package to the `message` package and rename it to `ZWaveJSValueNotificationEventMessage` for consistency with the other message types. Re-implement `ZWaveJSValueNotificationEventMessage` in terms of `EventMessage` and a new `ZWaveJSValueNotificationData` type that represents its data.
I've seen HA sometimes formatting timestamps as RFC 3339 strings, sometimes as fractional seconds since the epoch. Add a type `message.TimeStamp` that can handle either one.
Move `FireEventRequest` to the `messages` package and make it look more like other messages.
It's documented by Home Assistant.
Move the `services.Target` type to the `message` package, so that it can be used more broadly.
Move types related to calling services from the `internal/services` package to the `message` package: * `services.CallServiceMessage` → `message.CallServiceRequest` * `services.BaseServiceRequest` → `message.CallServiceData`
Replace `type.NotifyRequest` with a new `message.NotifyData` type. This type doesn't include the service name; instead, it should be passed separately to the `Notify.Notify()` method.
Replace `type.SetTemperatureRequest` with a new type, `message.SetTemperatureData`. Given the JSON annotations, there is no need for this type to have an explicit `ToJSON()` method.
mhagger
force-pushed
the
message-package
branch
from
e8eb789 to
94db109
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, message types are declared in multiple packages:
websocket,gomeassistant,types,internal/services, and maybe somewhere else that I forgot. Partly this is historical, but partly it is to avoid import cycles. For example, the individual services need to construct messages, and the messages need to include a "target", but the top-level package imports theinternal/servicespackage. Therefore,Targetand some other messages are forced to be implemented in thewebsocketpackage, which doesn't feel right. There are also inconsistencies about how message types are named and whether they embedBaseMessage.Let's move all of the message types to a new
messagepackage. Thispackage won't have dependencies on any other packages, so it can be
imported from anywhere. And lets define the various message types in a more uniform way.
I'm leaving this PR in draft mode because it is built on top of the
call-returns-resultbranch of #47, which hasn't been merged yet. If and when that PR is merged, I'll retarget this PR at the new tip ofmainand take it out of "draft" mode./cc @saml-dev in case you are interested of a preview of my planned next step.