Skip to content

build DiscordClient on top of IOC #1908

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.

Sign up for GitHub

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

Jump to bottom
Merged
akiraveliara merged 24 commits into from
Jun 2, 2024
Merged

build DiscordClient on top of IOC #1908

akiraveliara merged 24 commits into from
Jun 2, 2024

Conversation

@akiraveliara
Copy link
Member

@akiraveliara akiraveliara commented May 17, 2024 •
edited
Loading

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
Copy link
Member

@VelvetToroyashi VelvetToroyashi left a comment

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

DSharpPlus/Net/Rest/RestClient.cs
Comment on lines +74 to +75
this.httpClient.BaseAddress = new Uri(Utilities.GetApiBaseUri());
this.httpClient.Timeout = timeout;
Copy link
Member

@VelvetToroyashi VelvetToroyashi May 17, 2024

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)

Copy link
Member

@VelvetToroyashi VelvetToroyashi May 17, 2024 •
edited
Loading

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

Copy link
Member Author

@akiraveliara akiraveliara May 30, 2024

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
36 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
Copy link
Contributor

@OoLunar OoLunar left a comment

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.

VelvetToroyashi
VelvetToroyashi approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@VelvetToroyashi VelvetToroyashi left a comment

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
Copy link
Member Author

akiraveliara commented May 31, 2024

@OoLunar added the migration guide as requested

OoLunar
OoLunar approved these changes Jun 2, 2024
View reviewed changes
@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