Skip to content
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

Builder paradigm overhaul, second attempt #2024

Merged
merged 40 commits into from
Jul 24, 2022

Conversation

mkrasnitski
Copy link
Collaborator

After working for a while on #1967, I realized that there were problems with the approach I was taking, and that solving them would require rewriting a good portion of what had already been pushed. So, I'm going to take a fresh start and hopefully get it right this time. Again, the goal is reduce monomorphization bloat due to generic closure arguments, allow for re-using preconstructed builders, and pave the way for later changes down the line that bring the library more in line with Discord's official docs (e.g. enforcing required fields, implementing audit_log_reason, etc.).

Here is the new plan:

  1. Model methods will remain async and their return types will be unchanged, but they will now take a concrete Builder type as argument instead of a generic closure like FnOnce(&mut Builder) -> &mut Builder.
  2. To reduce code duplication, builders will have an execute method that performs the API request. This helps in cases where model methods duplicate functionality between Guild/PartialGuild/GuildId, or GuildChannel/ChannelId, etc. I am initially in favor of exposing this method to users, though we may want to mark it pub(crate) instead, depending on consensus.
  3. To allow for instantiating a builder inline via Builder::default().method1().method2(), methods on builders will take and return Self instead of &mut Self.

@github-actions github-actions bot added builder Related to the `builder` module. http Related to the `http` module. model Related to the `model` module. labels Jun 29, 2022
@github-actions github-actions bot added utils Related to the `utils` module. examples Related to Serenity's examples. framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate labels Jun 30, 2022
@mkrasnitski mkrasnitski force-pushed the builder_rework branch 7 times, most recently from 4933648 to eff57b6 Compare July 7, 2022 18:30
@mkrasnitski mkrasnitski force-pushed the builder_rework branch 3 times, most recently from 017b5c8 to 60b8799 Compare July 13, 2022 17:39
@mkrasnitski
Copy link
Collaborator Author

Well then, I think this PR is now ready for review. I've gone through and reworked everything in the builder directory. There are a few stray builders in the utils directory, specifically MessageBuilder and CustomMesage, but the former is so huge that probably it makes sense to tackle it separately, if at all. The latter isn't used anywhere in the codebase, so I'm not sure what to do with it. Is it actually used by downstream users?

As I've gone through, any extra breaking changes in each builder, such as removal of methods, or changing field types, have been noted in the corresponding commit messages. I could compile them all together if desired.

I think this should land after v0.11.3 is released, because there are some changes in current, as well as in currently-open PR's that I would like to rebase on top of.

@mkrasnitski mkrasnitski marked this pull request as ready for review July 14, 2022 18:38
@arqunis arqunis force-pushed the next branch 3 times, most recently from f06e6e7 to b6d6ce0 Compare July 20, 2022 02:43
This includes reworking `CreateGuildWelcomeChannel` as well, which means
removing `EditGuildWelcomeScreen::create_welcome_channel` as it is now
made redundant by the `add_welcome_channel` method.
Changed `CreateComponents::set_action_rows` to overwrite the list of
action rows, rather than extend it. Added `add_action_rows` to do that
instead.

Removed the following builder methods, as they became redundant:
  - CreateComponents::create_action_row
  - CreateActionRow::create_button
  - CreateActionRow::create_select_menu
  - CreateActionRow::create_input_text
  - CreateSelectMenuOptions::create_option

Also, replaced all `set_components` methods with just `components`.
Removed the following now-redundant methods:
  - CreateApplicationCommands::create_application_command
  - CreateApplicationCommand::create_option
  - CreateApplicationCommandOption::create_sub_option
  - CreateApplicationCommandsPermissions::create_application_command
  - CreateApplicationCommandPermissions::create_permissions
  - CreateApplicationCommandPermissionsData::create_permission

Also, made a note that `CreateApplicationCommandsPermissions` and the
methods that call it no longer work, due to the endpoint being disabled.
Also changed model methods on `Guild` and `PartialGuild` to take `&self`
instead of `self`.
@mkrasnitski
Copy link
Collaborator Author

mkrasnitski commented Jul 24, 2022

Rebased on next now that #2061 was merged, and also reworked the new EditAutoModRule builder and the new Webhook example. Should be all set for review.

EDIT: For some reason, github's listing of the commits above has some out of order/duplicated. I'd recommend just using the commits tab to get a list of all commits.

src/builder/create_application_command.rs Outdated Show resolved Hide resolved
Co-authored-by: Alex M. M. <acdenissk69@gmail.com>
@arqunis arqunis merged commit 5edbf0b into serenity-rs:next Jul 24, 2022
@mkrasnitski mkrasnitski deleted the builder_rework branch July 24, 2022 03:58
arqunis pushed a commit that referenced this pull request Aug 21, 2022
arqunis pushed a commit to arqunis/serenity that referenced this pull request Sep 2, 2022
kangalio pushed a commit to kangalio/serenity that referenced this pull request Sep 11, 2022
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 1, 2022
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Nov 7, 2022
arqunis pushed a commit that referenced this pull request Nov 13, 2022
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Feb 28, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request May 18, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request May 30, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Sep 21, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 17, 2023
mkrasnitski added a commit to mkrasnitski/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
arqunis pushed a commit to arqunis/serenity that referenced this pull request Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builder Related to the `builder` module. examples Related to Serenity's examples. framework Related to the `framework` and `framework::standard` modules and/or the `command_attr` crate http Related to the `http` module. model Related to the `model` module. utils Related to the `utils` module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants