Skip to content

build DiscordClient on top of IOC#1908

Merged
akiraveliara merged 24 commits into
masterfrom
aki/discordclient-ioc
Jun 2, 2024
Merged

build DiscordClient on top of IOC#1908
akiraveliara merged 24 commits into
masterfrom
aki/discordclient-ioc

Conversation

@akiraveliara

@akiraveliara akiraveliara commented May 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

current status:

  • RestClient capable of construction
  • Gateway incapable of construction
    • dispatch constructed and extensible through IOC
    • possibly the raw gw client extensible through IOC?
  • expand capability of DiscordClientBuilder to allow configuring logging and services
  • process events through DiscordClientBuilder
  • obsolete old events (on DiscordClient only, since this won't support sharding yet)
  • docs

non-goals of this PR:

  • sharded client support (future PR)
  • extension support (future PR)
  • exposing more extensibility interfaces (future PR, open issues for each)
  • ratelimiting constructed and extensible through IOC (future PR)

leave issues for any other features associated with this you may want

closes #1683
makes progress towards #1820

@akiraveliara akiraveliara added enhancement core epic-ioc for pull requests and issues relating to IOC in v5 labels May 17, 2024
@akiraveliara akiraveliara added this to the v5.0 milestone May 17, 2024
VelvetToroyashi
VelvetToroyashi reviewed May 17, 2024
View reviewed changes

@VelvetToroyashi VelvetToroyashi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from comments; gonna open an issue wrt some tomfoolery happening regarding ratelimit handling

Comment thread DSharpPlus/Extensions/ServiceCollectionExtensions.InternalSetup.cs
Comment thread DSharpPlus/Net/Rest/RestClient.cs
Comment on lines +74 to +75
this.httpClient.BaseAddress = new Uri(Utilities.GetApiBaseUri());
this.httpClient.Timeout = timeout;

@VelvetToroyashi VelvetToroyashi May 17, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it'd be worthwhile to go the Remora route and expose a [rest] client builder; there's occasional reason to change what the client does (e.g., adding additional polly handlers for metrics, or using the canary api for one reason or another)

@VelvetToroyashi VelvetToroyashi May 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Client builder as in Action<HttpClient> parameter)

https://github.com/Remora/Remora.Discord/blob/87adfc57b1a0980234713f6c77a1e2be25f5567e/Backend/Remora.Discord.Gateway/Extensions/ServiceCollectionExtensions.cs#L54-L59

@akiraveliara akiraveliara May 30, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is something we should look into in a separate pull request to customize specifically rest further

@VelvetToroyashi VelvetToroyashi mentioned this pull request May 17, 2024
Closed
@akiraveliara akiraveliara mentioned this pull request May 21, 2024
Open
31 tasks
@Plerx2493 Plerx2493 mentioned this pull request May 25, 2024
Closed
@akiraveliara akiraveliara marked this pull request as ready for review May 29, 2024 19:47
OoLunar
OoLunar approved these changes May 30, 2024
View reviewed changes

@OoLunar OoLunar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is okay? Regardless please write a migration guide for this PR specifically. Before we release v5, we'll merge the migration guide for this PR and all other breaking changes into one. I just want a guide to reference right now since I myself don't particularly understand what the intended use is.

Comment thread DSharpPlus/Clients/DiscordClient.Dispatch.cs
Comment thread DSharpPlus/Clients/DiscordClient.Events.cs
Comment thread DSharpPlus/Clients/DiscordClient.cs Outdated
VelvetToroyashi
VelvetToroyashi approved these changes May 31, 2024
View reviewed changes

@VelvetToroyashi VelvetToroyashi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursory glance (namely because of a jumble of a diff), but LGTM.

@akiraveliara

akiraveliara commented May 31, 2024

Copy link
Copy Markdown
Member Author

@OoLunar added the migration guide as requested

OoLunar
OoLunar approved these changes Jun 2, 2024
View reviewed changes
Comment thread docs/articles/migration/4x_to_5x.md Outdated
Comment thread docs/articles/migration/4x_to_5x.md Outdated
@akiraveliara akiraveliara merged commit 613936b into master Jun 2, 2024
@akiraveliara akiraveliara deleted the aki/discordclient-ioc branch June 2, 2024 19:09
This was referenced Jun 3, 2024
Merged
Open
@akiraveliara akiraveliara mentioned this pull request Jul 7, 2024
Merged
@ecrocombe ecrocombe mentioned this pull request Jul 9, 2024
Closed
OoLunar pushed a commit that referenced this pull request Oct 3, 2024
@akiraveliara
build DiscordClient on top of IOC (#1908)
44a40d8 
* centralize the service provider

* prepare the rest client for IOC

* obsolete all the old events...

* obsolete SocketErrored even more

* enables registering event handlers to the service collection

* fix event-related build errors

* make DiscordApiClient IOC-constructible

* death (migrate events (not dispatch yet) to the interim model)

* update DiscordClient.Dipshit.cs

* fix most trivial build errors

* more progress towards usability

* DSharpPlus.dll should now work:tm:

* build?

* miser, miser

* add public construction code

* update docs

* convenience

* details

* migration guide (partial, only as far as this PR is concerned)

* build

* switch to CreateDefault

* nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@VelvetToroyashi VelvetToroyashi VelvetToroyashi approved these changes
@Plerx2493 Plerx2493 Awaiting requested review from Plerx2493
+1 more reviewer
@OoLunar OoLunar OoLunar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

big-change core docs enhancement epic-ioc for pull requests and issues relating to IOC in v5

Projects

None yet

Milestone

v5.0

Development

Successfully merging this pull request may close these issues.

Centralized ServiceProvider

3 participants

@akiraveliara @VelvetToroyashi @OoLunar