Turn embed into a behaviour #201
Conversation
There was a problem hiding this comment.
Thanks for the PR! I definitely don't hate this idea and it doesn't seem like it'd cause any harm if it were to be added. Additionally there must be a use case if you're suggesting adding this capability. However, I do have some feedback about it!
I'll be the first to admit I haven't looked closely at behaviors in the past or how they're implemented, so feel free to correct me if I'm wrong on anything. To me it feels like this implementation is too closely coupled to the actual struct module itself, and I'm not sure how we'd fix that.
Take the author field for example. The author that their implementation returns needs to have the correct fields (name, url, icon_url). Do you think this is clear? The only indicator is the type spec which now that I'm thinking about it might be enough?
Additionally, if these fields don't exist the user will not get a useful error. For example, if their author/1 method doesn't return a term with the url field they'll get an error that :url doesn't exist in their term. IMO this would be unfriendly inside of a function doing as much as from/1 is.
lib/nostrum/struct/embed.ex
Outdated
| @@ -381,6 +422,46 @@ defmodule Nostrum.Struct.Embed do | |||
| put_field(%__MODULE__{embed | fields: []}, name, value, inline) | |||
| end | |||
|
|
|||
| @doc """ | |||
| Create an embed from a struct that implements the protocol `Nostrum.Struct.Embed.Protocol`. | |||
There was a problem hiding this comment.
I don't think I'm 100% happy with the wording here. Structs don't necessarily implement a protocol per se, right? It's the module that implements the behavior. I obviously get what you're trying to say but it's not 100% correct and if someone is unfamiliar with behaviors I could see there being issues.
Also, this still mentions a protocol when we're using a behavior now.
There was a problem hiding this comment.
My bad. I wrote this while I still relied on protocols and forgot to update it 🙏
|
Elixir LS should pick up on type mismatching in this case and emit a warning, but we can always add strict pattern matching and error messages to further help devs |
|
Sorry for the late response, I'm happy to move forward with this. I do think I would like to include the mentioned pattern matching and/or better error messages! |
|
Added some pattern matching for the callbacks that return structs (author, footer and fields). I wasn't sure whether wanted a check for all other fields as well, so I omitted it for now. |
|
This looks good! I think some short examples would be nice, but besides that I'd be happy to merge it! |
|
While adding some short examples with with doc tests I noticed that I call |
|
Thanks! |
* Turn embed into a behaviour * Adjust type specs * defmacro * consistency * Fix wording * Add type check for callbacks * handle nil * add example in documentation and tests Co-authored-by: Awlex <no thank you>
* Turn embed into a behaviour * Adjust type specs * defmacro * consistency * Fix wording * Add type check for callbacks * handle nil * add example in documentation and tests Co-authored-by: Awlex <no thank you>
Basic implementation. If this approach is acceptable I'll add documentation about the usage, before merging.