-
Notifications
You must be signed in to change notification settings - Fork 129
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
Api Correction Part 2: Emojis #48
Conversation
lib/nostrum/api.ex
Outdated
|
||
Returns `{:ok}` if successful, `{:error, reason}` otherwise. | ||
Nostrum.Api.create_reaction(123123123123, 321321321321, "\xF0\x9F\x98\x81") |
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.
For these examples, I think they should be in code blocks, probably with proper syntax highlighting.
In the elixir docs, they often use the iex>
example format within code blocks. This was something we did in the last PR, sans the codeblock (which should be rectified). I don't know if we need to include the iex>
portion as we're not showing the outcome, but I think codeblocks would be better than just markdown.
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.
The examples were already code-blocked due to four-space indentation, but I've gone ahead and added some explicit triple-tick markdown for the examples.
I decided to go ahead and remove iex>
from the examples I've pushed. IIRC, iex>
should only be used whenever a function can be tested without server or side effects. Source: https://hexdocs.pm/elixir/master/writing-documentation.html#doctests
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 actually didn't know about the four-space indentation. Do they auto highlight to elixir syntax? If so then it's not a big deal going forward.
And oh yeah, duh, we didn't have any doctests prior and I forgot about them. Given that is the case, I think it should be removed from modify_current_user/1
.
lib/nostrum/api.ex
Outdated
Returns `{:ok}` if successful, `{:error, reason}` otherwise. | ||
Nostrum.Api.create_reaction(123123123123, 321321321321, "\xF0\x9F\x98\x81") | ||
|
||
Nostrum.Api.create_reaction(123123123123, 321321321321, URI.encode("\u2b50")) |
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.
Maybe we should also include an example of using Emoji.to_api_name/1
here?
Also, I don't think it's necessary to include these two examples in other functions. If we want we could just have those functions point towards the example here.
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've added an example with Emoji.to_api_name/1
, as well as made the other emoji functions point to create_reaction/3
for examples. Let me know how it looks.
lib/nostrum/struct/emoji.ex
Outdated
@doc """ | ||
Formats an emoji struct into its `t:Nostrum.Struct.Emoji.emoji_api_name/0`. | ||
|
||
## Examples |
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.
Do you think it would be useful to include an example of using this function with an api function here? I know I asked to include one in the api module itself, but maybe it'd help?
I'll leave it up to you.
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 I think it'd be useful. Added one with latest commit.
lib/nostrum/struct/emoji.ex
Outdated
"<:foxbot:43819043108>" | ||
""" | ||
@spec to_markdown(t) :: String.t | ||
def to_markdown(emoji) |
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.
For these two functions, do we want to include a function definition that bypasses having to supply a struct?
I'm thinking just a 2 arity function of the same name that takes an id
and a name
. Or maybe a 3 arity with default animated set to false
?
Maybe the use case is kind of niche? Let me know what you think.
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.
Seems niche IMO. Even if I were to add it, I'd prefer to create a new module for that. Functions in Nostrum.Struct.Emoji
should explicitly deal with the Emoji struct and nothing else IMO. I'll think about it.
With that being said, I'll probably allow maps (in addition to the Emoji struct) to work with this function. Just now, I realized that parts of nostrum's payload data isn't properly decoded to structs. Functions like these will break if I don't allow maps to work.
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.
If the payload isn't being decoded properly (which is the case a lot of the time, probably), then that is an issue and I don't think we should support that. I partly agree with your idea that we should keep it limited to functions dealing with Emoji structs. If we're going down this route we should stick to it and not allow maps.
I could see users already knowing the id and name and wanting to bypass having to create a struct, but in that case I guess they can just form their own string?
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'll revert the map changes then.
For the idea you proposed with having an id
and name
, I have a solution, but it might be hard to implement right now. It also suggests having a rather strong reliance on using structs in the project:
In my current project, I currently bind emoji markdown to function names for ease of access:
defmodule Emojis do
def unranked, do: "<:unranked:425713028852350976>"
def silver, do: "<:silver:425713020862332930>"
end
# Example
"This text will be sent with this emoji: #{Emojis.silver()}"
"This text will be sent with this emoji: <:silver:425713020862332930>"
I was thinking in the future, we could have something like an EmojiCache
to select emojis. Assume EmojiCache.get/1
accepts a keyword list and returns a Nostrum.Struct.Emoji
or nil
:
defmodule Emojis do
alias Nostrum.Cache.EmojiCache
def unranked, do: EmojiCache.get(name: "unranked", id: 425713028852350976)
def silver, do: EmojiCache.get(name: "silver", id: 425713020862332930)
end
# Example
Emojis.silver()
%Nostrum.Struct.Emoji{
id: 425713020862332930,
name: "silver",
roles: []
}
# Assuming `Nostrum.Struct.Emoji` will implement the `String.Chars` protocol,
# this will be possible:
"This text will be sent with this emoji: #{Emojis.silver()}"
"This text will be sent with this emoji: <:silver:425713020862332930>"
# As well as pulling information wherever necessary:
Emojis.silver().name
"silver"
Emojis.silver().roles
[]
Having an EmojiCache
would also open users up to error handling in case an emoji is not there:
alias Nostrum.Cache.EmojiCache
case EmojiCache.get(id: 4839104319) do
nil -> "\:silver\:" # In case the emoji got deleted
emoji -> emoji
end
While this next positive might be beyond the scope of nostrum, we could also open up this EmojiCache
to fetch an emoji via markdown:
"This text will be sent with this emoji: <:silver:425713020862332930>"
|> String.split(" ")
|> Enum.each(fn token ->
case token do
"<:" <> _rest = emoji_token ->
EmojiCache.get(markdown: emoji_token)
_ ->
:noop
end
end)
Let me know your thoughts. For the most part, I think this explains how I was thinking of going about doing this. It's a rather struct-heavy idea, but I think it would fit the use-cases of the project fairly well.
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 don't think this is a bad idea.
My issue is how we go about this with other modules. Take a module which has permissions for example. Undoubtedly we'll have some way of interacting with those, likely in a way in which supplying a struct wouldn't make sense. In that case, I don't think a similar approach would work.
What do you think?
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.
If I'm understanding correctly, I do not see this a problem. Discord permissions are usually bound to some kind of discord object (channel, role, overwrite, etc). I don't think there would be a scenario where we would have a cache that shares individual permissions. I may need more context.
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 wasn't thinking of needed a cache for the permissions. I meant to say that we'll probably want some helper methods around permissions, and I could see those taking in some extraneous parameters? I guess in that case you'd still want the discord object struct to perform the operation on, so it'd be okay.
I guess I can't think of anything off the top of my head as an example. In the case of permissions we might just want a different module that handles any logical operations on the subject? We can handle this when we get there.
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, I had the same idea with the permissions. Ideally, we'd create a module for the permissions that has functions for working with them. This would be similar to the Nostrum.Struct.Snowflake
module.
lib/nostrum/struct/emoji.ex
Outdated
Nostrum.Struct.Emoji.to_markdown(emoji) | ||
"<:foxbot:43819043108>" | ||
""" | ||
@spec to_markdown(t) :: String.t |
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.
Correct me if I'm wrong, but the purpose of this here is for sending emojis in messages, yeah? It might be a good idea to state this or include an example of doing so.
As it stands I think to_markdown
can be a little vague.
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'll add an example in this upcoming commit.
I think to_markdown
is vague as well. Maybe format
would be a better name? Also, I was actually thinking about having the String.Chars
protocol handle the markdown conversion. This would allow the emojis to be used in strings in a more natural method:
emoji = %Nostrum.Struct.Emoji{id: 4314314, name: "craigpls"}
Kernel.to_string(emoji)
"<:craigpls:4314314>"
"I get to use this emoji now #{emoji}"
"I get to use this emoji now <:craigpls:4314314>"
And then we can always leave a markdown conversion function in case the user wanted a more explicit function call:
emoji = %Nostrum.Struct.Emoji{id: 4314314, name: "craigpls"}
md = Nostrum.Struct.Emoji.format(emoji)
"<:craigpls:4314314>"
"Got some new text #{md}"
"Got some new text <:craigpls:4314314>"
Let me know your thoughts.
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 think format
or format_emoji
would be good choices.
I like the protocol idea, lets go with that! Just make sure its documented properly.
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.
This next commit will reflect these changes. I've used format
in place of to_markdown
.
lib/nostrum/struct/emoji.ex
Outdated
@typedoc "User that created this emoji" | ||
@type user :: User.t | nil | ||
|
||
@typedoc "Whether this emoji must be wrapped in colons " |
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.
Extra space here at the 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.
Fixed in latest commit.
|
Will rename it in this upcoming commit.
I tried using a name like
It's axing time :^). |
lib/nostrum/api.ex
Outdated
|
||
Nostrum.Api.delete_own_reaction(123123123123, 321321321321, URI.encode("\u2b50")) | ||
|
||
See `create_reaction/3` for similar examples. |
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 don't think we need the Examples
sub-header if we're going to point to another function like this.
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.
Removed in latest commit.
I'll be changing the PR to [WIP] for now. I realized that there are a few Emoji endpoints that haven't been added to the API. In particular:
I'm going to do those and comment back here when I get those done. |
Okay I've added a couple of things now with this last push:
Notes:
There's nothing in the normal Elixir library that allows users to build these from an image. Might be a good idea to not require users to do this. Here is how I've been building them to test: bytes = File.read!("/dir/here/image.png")
base_encode = Base.encode64(bytes)
data_uri = "data:image/png;base64," <> base_encode
Nostrum.Api.create_guild_emoji(431890431431, name: "emoji", image: data_uri) |
lib/nostrum/struct/emoji.ex
Outdated
end | ||
end | ||
|
||
defimpl String.Chars, for: Nostrum.Struct.Emoji do |
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 think we should move this into the module definition.
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.
Agreed. Added in the upcoming commit.
lib/nostrum/api.ex
Outdated
|
||
`:name` and `:image` are always required. | ||
|
||
## Return Values |
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've noticed that when there are options like you've included this sub-section. I don't think I'm a big fan of having it be its own section, but I'm open for discussion. We should decide how we want to display this, and make sure it's homogeneous throughout the module.
The elixir docs are kind of unorganized in how they do this, probably because their functions are a lot less linear than ours.
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.
The "Return Value" header is there to distinguish the text from the options. If we want to remove this header, I think it would be best to remove the "Options" header as well. What do you think?
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.
Do you think it'd be okay to move under the original doc definition? I just feel it's out of place since every method has results (while not every method has optional params), but the section is only included on those methods that have optional params. I feel like that could be confusing?
A lot of times I point to the elixir docs just to see how they do it. Here for example they fit it in pretty smoothly.
While at the same time from the last pr we have places that miss returning the value altogether.
I think it might be worth it to hash out a design or some sort of template (maybe through the discord channel for quicker iteration) we could use to kind of bring all of these docs in line. And then going forward we can follow that and I'll go through and fix up the previous PR if need be.
lib/nostrum/api.ex
Outdated
@spec create_reaction(integer, integer, String.t | Nostrum.Struct.Emoji.custom_emoji) :: error | {:ok} | ||
def create_reaction(channel_id, message_id, emoji) do | ||
request(:put, Constants.channel_reaction_me(channel_id, message_id, emoji)) | ||
@spec create_reaction(Channel.id, Message.id, Emoji.t | Emoji.emoji_api_name) :: error | {:ok} |
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.
Maybe we should include an emoji
type at the top of this module that is comprised of this definition here? Namely the Emoji.t | Emoji.emoji_api_name
portion.
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.
Agreed. I'll be adding that in this upcoming commit.
lib/nostrum/api.ex
Outdated
Nostrum.Api.create_guild_emoji(43189401384091, name: "nostrum", image: image, roles: []) | ||
``` | ||
""" | ||
@spec create_guild_emoji(Guild.id, keyword | map) :: error | {:ok, Emoji.t} |
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.
Hmm, throughout this module we'll have keyward | map
in specs whenever there are optional parameters. Do you think we should similarly include something like an optional
type that describes the map/keyword list functionality we have here?
If we decide to do something like this feel free to open an issue so we can keep track of it. We probably don't want to go back to the user
functions in this pr and change them.
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'll create an issue specifically for tracking these. However, I'll be changing the keyword | map
types for the current Emoji-related endpoints.
I'm not sure I'm keen on including the functionality you described. Our API is relatively lower level as it stands, and this I think might be a little out of place? I could maybe see the argument for the inclusion of the data URI information (with some work), but even then having an example may be sufficient, maybe a link to some proper docs on the subject? |
Now that I think about it, I think providing examples on data URI related functions will already give the library user an idea of what to work with. I'd say the examples give enough information for the user to google up if they are struggling. We probably don't need to include something like this in nostrum. |
After our discord discussion with how the documentation should be handled, I've updated the documentation to all functions relating to this specific PR. We will have to go back sometime to update the functions we didn't get to. I also added in another banged function. |
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.
Last couple things and this will be ready to merge!
lib/nostrum/api.ex
Outdated
This endpoint requires the `VIEW_CHANNEL` and `READ_MESSAGE_HISTORY` | ||
permissions. Additionally, if nobody else has reacted to the message with | ||
the `emoji`, this endpoint requires the `ADD_REACTIONS` permission. It | ||
fires the `MESSAGE_REACTION_ADD` event. |
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.
When talking about what events are fired, do you think we should link to the event in the Consumer
module? Their type is listed there.
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.
For now, I think this is a good idea. Going to put these in the next commit. I said for now, because I remembered you mentioned something about removing the Consumer module.
lib/nostrum/api.ex
Outdated
Nostrum.Api.create_reaction(123123123123, 321321321321, "\xF0\x9F\x98\x81") | ||
|
||
# Using a URI encoded emoji string. | ||
Nostrum.Api.create_reaction(123123123123, 321321321321, URI.encode("\u2b50")) |
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've made encoding the URL the default behavior, so this is no longer necessary (and actually wasn't in the first place).
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.
Removed in latest commit.
lib/nostrum/struct/emoji.ex
Outdated
# Unicode Emojis | ||
"👍" | ||
"\xF0\x9F\x98\x81" | ||
URI.encode("\u2b50") |
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.
We don't need the URI.encode
here anymore, but I think it's a good example of using codepoints w/ the sigil so I think the rest of the example can stay here.
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.
Removed in latest commit.
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.
Could you rebase this pr to clean it up a bit before we merge, just like you did with the last pr? Thanks!
lib/nostrum/struct/emoji.ex
Outdated
@@ -61,7 +60,6 @@ defmodule Nostrum.Struct.Emoji do | |||
# Unicode Emojis | |||
"👍" | |||
"\xF0\x9F\x98\x81" | |||
URI.encode("\u2b50") |
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.
Sorry if I wasn't clear. Could we still have the example here just without the URI.encode
function call?
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.
Oops. Fixed in latest commit.
Updated docs on `create_reaction/3` in Api module Added bangified `create_reaction!/3` to Api Fixed docs of `delete_own_reaction/3` in Api Added bangified `delete_own_reaction!/3` to Api Fixed docs of `delete_reaction/4` in Api Added bangified `delete_reaction!/4` in Api Updated `get_reactions/3` emoji types in Api
Moved documentation of emoji_api_names into Emoji module Added examples for `to_markdown/1` in Emoji module Added examples for `to_api_name/1` in Emoji module Removed extraneous parenthesis from `delete_own_reaction!/3` Added examples to `get_reactions/3` in Api Encapsulated doc examples in explicit markdown Simplified emoji-related Api examples Improved `create_reaction/3` examples. Made other emoji functions point to `create_reaction/3`. Added example to `to_api_name/1` Allowed maps to work in `to_api_name/1` and `to_markdown/1` Added Api example to `to_markdown/1` Removed space from typedoc.
Renamed `delete_reaction/4` to `delete_user_reaction/4` Removed `format_custom_emoji/2` function in Emoji Fixed documentation bug regarding string interpolation in examples
Allowed only Emoji structs to work with `to_api_name/1` and `to_markdown/1` Renamed `to_markdown/1` to `format/1`
Added `String.Chars` protocol to `Emoji` Removed example headers from emoji Api fns for clarity Added docs for using the Emoji struct in messages and api Renamed `format/1` to `format_mention/1` Renamed `to_api_name/1` to `get_api_name/1` Allowed Emoji structs to work with Create Reaction endpoint Allowed Emoji structs to work with Delete Own Reaction endpoint Allowed Emoji structs to work with Delete User Reaction endpoint Allowed Emoji structs to work with Get Reactions endpoint
Added List Guild Emojis endpoint to Api Added Get Guild Emoji endpoint to Api Added Create Guild Emoji endpoint to Api Added Modify Guild Emoji endpoint to Api Added Delete Guild Emoji endpoint to Api
Trying new options format for Api functions
Moved `String.Chars` protocol into Emoji module Added emoji type in Api Added options type in Api Updated emoji and reaction documentation to fit standards Added bangified function to Delete All Reactions endpoint
Updated api events to link to consumer event type Removed URI emoji encoding in docs Added unicode emoji example Space ending fixes
54b9dd1
to
e8d57c0
Compare
Rebased and squashed :^) |
🎉 🎆 🎉 🎆 |
❤️ 💚 💙 💛 💜 |
This PR focuses on correcting the Api by cleaning up the pre-existing structs.
This PR does the following:
Notes:
delete_reaction/4
in the Api does not have a matching name with the corresponding Discord Api endpoint (DELETE USER REACTION). This is a minor problem, but we could try renaming this function to match it with the endpoint name.to_api_name/1
andto_markdown/1
in the Emoji module to help lib users use Emoji structs with the Api. I'm thinking of potentially renaming these toget_api_name/1
andformat_markdown/1
. Up for discussion.format_custom_emoji/2
in the Emoji module does not seem to be useful from my perspective. Potential deprecation or removal?custom_emoji
type in favor ofemoji_api_name
. I think the latter is more representative of the type's purpose. It also lets us cover the api names of all emojis (not just custom ones).TODO: