-
Notifications
You must be signed in to change notification settings - Fork 69
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 type information for inbound messages on the DeviceChannel #1444
base: main
Are you sure you want to change the base?
Conversation
ca40d3a
to
a596340
Compare
I was looking over the spec for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dug into the serializer and more about the Phoenix.Channel
behavior based on Josh comment and I'm unsure what to about it. There seems to be lots of partial enforcement of string and not-string. It seems we don't get much out of converting atoms and types at that point since most everything about the channel is overly generic.
The main gain is the documentation of messages. I think that is going to be split between server and extensions, so I'm not sure the best way to represent yet
@@ -190,7 +190,7 @@ defmodule NervesHub.Deployments do | |||
archive_id: deployment.archive_id | |||
} | |||
|
|||
broadcast(deployment, "archives/updated", payload) | |||
_ = broadcast(deployment, "archives/updated", payload) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this is a dialyzer fix? We should prob include this in a different PR so its away from these changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, dialyzer
types | ||
|> Enum.map(fn type -> | ||
try do | ||
String.to_existing_atom(type) | ||
rescue | ||
_ -> nil | ||
end | ||
end) | ||
|> Enum.filter(fn type -> | ||
if type in @valid_types do | ||
true | ||
else | ||
Logger.warning("Received invalid type for connection_types: #{inspect(type)}") | ||
false | ||
end | ||
end) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types | |
|> Enum.map(fn type -> | |
try do | |
String.to_existing_atom(type) | |
rescue | |
_ -> nil | |
end | |
end) | |
|> Enum.filter(fn type -> | |
if type in @valid_types do | |
true | |
else | |
Logger.warning("Received invalid type for connection_types: #{inspect(type)}") | |
false | |
end | |
end) | |
for {type, type_str} <- Ecto.Enum.mappings(Device, :connection_types), type_str in types, do: type |
We can use the Ecto.Enum mappings to get the valid types out of this. I'm unsure we need to warn on invalid being included? (this would mostly be hidden to the users anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just figured we'd do something when we see weird things :)
Would Ecto.Enum be smoother somehow or are you thinking full-on embedded schema?
So with |
No description provided.