API module: The Phantom Menace

[jb3]

When I open api.ex, my Emacs lags. This is simply unacceptable, as Emacs is known to be blazingly fast. I pondered, why could this be, why would my Emacs have anything above mere milliseconds of delay on opening a file. Then it hit me.

$ wc -l lib/nostrum/api.ex
4479 lib/nostrum/api.ex

I have decided that this will not do, and that we must scope out all methods to respective sub-modules. In the words of Craig:

I have hence decided to form an opinionated list of how we should do this restructure to kick off the discussion process, I encourage as much feedback as we can get here. I used Nostrum.Api.__info__(:functions) |> Keyword.keys |> Enum.uniq to get this list.

The convention I have used below is the newly scoped module, then the current name (under Nostrum.Api) and the post-move module name.

Thoughts

Migration

Would be very cool to try make this migration as simple as possible, maybe looking at custom Credo checks or other utilities to scan for usages of old Nostrum.Api calls or aliases and recommending the changes.

Bangified methods

At the same time we make this change, we should ensure that all methods have a bangified equivalent if they can potentially return an API error.

I also wish there was a better way to do bangified functions, or to offload that to the user, because it makes the API functions list twice as complicated as it needs to be. In my mind, I want to shove bangify to the user-side and let them upgrade things to exceptions, but I don't know if that's as idiomatic/a good idea.

Thoughts on this greatly appreciated, I wish we could do away with them, (and frankly, we already are missing enough of them that I think users would find ways to just handle things better)

Food for thought: Constants

I would also like to ask the question of how we handle the Constants module and if that also requires any form of refactoring after these changes.

Guild module

Slightly related, but since the proposed Guild module is huge, I'd like to suggest:

• Adding sticker API methods and moving Stickers/Emojis into one module (probably Nostrum.Api.Expressions, like the API calls it)
• Removing pointless methods like create_guild_integrations, it's not even a documented route

Other notes

I have only looked at public functions in the Api module, there are a lot of private internals that would need to be figured out.

The Modules

Nostrum.Api

request => request request_multipart => request_multipart

Nostrum.Api.ApplicationCommands

batch_edit_application_command_permissions => batch_edit_permissions bulk_overwrite_global_application_commands => bulk_overwrite_global_commands bulk_overwrite_guild_application_commands => bulk_overwrite_guild_commands create_global_application_command => create_global_command create_guild_application_command => create_guild_command delete_global_application_command => delete_global_command delete_guild_application_command => delete_guild_command edit_application_command_permissions => edit_command_permissions edit_global_application_command => edit_global_command edit_guild_application_command => edit_guild_command get_application_command_permissions => get_command_permissions get_global_application_commands => get_global_commands get_guild_application_command_permissions => get_guild_command_permissions get_guild_application_commands => get_guild_commands

Nostrum.Api.AutoModeration

create_guild_auto_moderation_rule => create_auto_moderation_rule delete_guild_auto_moderation_rule => delete_auto_moderation_rule get_guild_auto_moderation_rule => get_auto_moderation_rule get_guild_auto_moderation_rules => get_auto_moderation_rules modify_guild_auto_moderation_rule => modify_auto_moderation_rule

Nostrum.Api.Channel

add_pinned_channel_message => pin bulk_delete_messages => bulk_delete_messages create_guild_channel => create delete_channel => delete delete_channel_permissions => delete_permissions delete_pinned_channel_message => unpin edit_channel_permissions => edit_permissions get_channel => get get_channel_messages => get_messages get_channel_webhooks => get_webhooks get_pinned_messages => get_pins modify_channel => modify start_typing => start_typing

Nostrum.Api.Guild

add_guild_member => add_member begin_guild_prune => begin_prune create_guild_ban => ban_member create_guild_emoji => create_emoji create_guild_integrations => create_integration delete_guild => delete delete_guild_emoji => delete_emoji delete_guild_integrations => delete_integration get_guild => get get_guild_audit_log => get_audit_log get_guild_ban => get_ban get_guild_bans => get_bans get_guild_channels => get_channels get_guild_emoji => get_emoji get_guild_integrations => get_guild_integrations get_guild_member => get_member get_guild_prune_count => estimate_prune_count get_guild_roles => get_roles get_scheduled_events => get_scheduled_events get_guild_webhooks => get_webhooks get_guild_widget => get_widget get_voice_region => get_voice_region leave_guild => leave list_guild_emojis => list_guild_emojis list_guild_members => get_members list_voice_regions => list_voice_regions modify_current_user_nick => modify_self_nick modify_guild => modify modify_guild_channel_positions => modify_channel_positions modify_guild_emoji => modify_emoji modify_guild_integrations => modify_integrations modify_guild_member => modify_member modify_guild_role_positions => modify_role_positions modify_guild_widget => modify_widget remove_guild_ban => unban_member remove_guild_member => kick_member sync_guild_integrations => sync_integrations

Nostrum.Api.Interactions

create_followup_message => create_followup_message create_interaction_response => create_response delete_interaction_followup_message => delete_followup_message delete_interaction_response => delete_response edit_interaction_response => edit_response get_original_interaction_response => get_original_response

Nostrum.Api.Invites

get_invite => get delete_invite => delete_invite get_guild_invites => get_guild_invites create_channel_invite => create get_channel_invites => get_channel_invites

Nostrum.Api.Message

create_message => create create_reaction => react delete_all_reactions => clear_reactions delete_message => delete delete_own_reaction => unreact delete_reaction => delete_reaction delete_user_reaction => delete_user_reaction edit_message => edit get_channel_message => get get_reactions => get_reactions

Nostrum.Api.Polls

expire_poll => expire get_poll_answer_voters => get_answer_voters

Nostrum.Api.Role

add_guild_member_role => add_member create_guild_role => create delete_guild_role => delete modify_guild_role => modify remove_guild_member_role => remove_member

Nostrum.Api.ScheduledEvent

create_guild_scheduled_event => create delete_guild_scheduled_event => delete get_guild_scheduled_event => get get_guild_scheduled_event_users => get_users modify_guild_scheduled_event => modify

Nostrum.Api.Self

get_application_information => get_application_information get_current_user => get get_token => get_token get_user_dms => get_dms modify_current_user => modify update_shard_status => update_shard_status update_status => update_status update_voice_status => update_voice_status

Nostrum.Api.Thread

add_thread_member => add_member get_thread_member => get_member get_thread_members => get_members join_thread => join leave_thread => leave list_guild_threads => list_threads list_joined_private_archived_threads => list_joined_private_archived_threads list_private_archived_threads => list_private_archived_threads list_public_archived_threads => list_public_archived_threads remove_thread_member => remove_member start_thread => create start_thread_in_forum_channel => create_in_forum start_thread_with_message => create_with_message

Nostrum.Api.User

create_dm => create_dm create_group_dm => create_group_dm get_current_user_guilds => get_current_user_guilds get_user => get get_user_connections => get_user_connections

Nostrum.Api.Webhook

create_webhook => create delete_webhook => delete edit_webhook_message => edit_message execute_git_webhook => execute_git execute_slack_webhook => execute_slack execute_webhook => execute get_webhook => get get_webhook_message => get_message get_webhook_with_token => get_with_token modify_webhook => modify modify_webhook_with_token => modify_with_token

[Kraigie]

+1 for removing the bangified methods. The only use case I can see for them is if you want some process to die if the function fails. That's simple enough for a user to do by themselves, and the benefit of being able to clean all of this up would be welcome.

I think it's a bad idea (probably), but want to point out the possibility of putting these (and other) functions in broader context modules. Think like Nostrum.Guild, as opposed to the suggested Nostrum.Guild.Api`. We'd probably want to move the structs into those modules as well. Kinda like it, kinda feel it might make things worse. Not sure.

As for the constants module, I'd opt to leave it as is. There's a lot of things in there that are scoped such that they wouldn't fit under any of the suggested modules, and I'm not sure there's a lot to be gained from such an internal facing module.

Good stuff!

[kshannoninnes]

I like it. I second the suggestion to move functions into Nostrum.Guild over Nostrum.Api.Guild. i subscribe to the ideology that a class (or in elixirs case a module) should only change when it's responsibility changes. I don't like the idea of changing the API module when the way guilds work is changed. The API module should only need to change if the actual discord API does.

I think I saw mention of using delegates for this in the discord, which I think is a good idea and, imo, will help keep the API module clean.

(Edit: to be clear I think retaining the existing API module and using it to delegate to the implementations is the right play, as it prevents this being a breaking change. The API module can later be deprecated in a major release)

[jchristgit]

I am in favour of these as well. I agree delegates are probably a good idea to keep it compatible for now.

One thought I have. Should we change get_something methods, e.g. get_bans(guild_id) to just bans(guild_id)? I think it's more idiomatic if we keep the get prefix as we're fetching from an external API, but I'm open to thoughts.

[jb3]

Agree on this, that is more idiomatic, I'll rewrite later today.

[jb3]

I do think to save the hellish possibility of 4k+ lines of new code as well as maintaining a huge api.ex file we really should just drop these methods for 1.0, but I'm open to looking more into the delegates.