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

Make Task.logger accessible to delegate implementations outside of Package #587

Merged

Conversation

felixschlegel
Copy link
Contributor

Motivation:

This change was proposed in issue #389.

Users writing their own HttpClientResponseDelegate implementation
might want to emit log messages to the task's logger.

Modifications:

Changed the access level of Task's logger property from internal
to public.

Result:

Users can access the logger property of a Task outside of the
async-http-client Package.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

6 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Collaborator

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

@fabianfett Would love to get your opinion on this one!

@@ -565,9 +565,9 @@ extension HTTPClient {
public final class Task<Response> {
/// The `EventLoop` the delegate will be executed on.
public let eventLoop: EventLoop
public let logger: Logger // We are okay to store the logger here because a Task is for only one request.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a simple doc comment to this property.

@felixschlegel felixschlegel force-pushed the fs-task-logger-access-level branch 2 times, most recently from e277c8d to 1e901b9 Compare May 24, 2022 14:28
@fabianfett fabianfett requested a review from glbrntt July 15, 2022 15:32
Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

LGTM. However we should habe @glbrntt way in as well.

@Lukasa Lukasa added the semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Jul 16, 2022
@glbrntt
Copy link
Collaborator

glbrntt commented Jul 20, 2022

@swift-server-bot add to allowlist

@FranzBusch FranzBusch linked an issue Jul 20, 2022 that may be closed by this pull request
@fabianfett
Copy link
Member

@swift-server-bot test this please

…ckage

Motivation:

This change was proposed in issue
[swift-server#389](swift-server#389).
Users writing their own `HttpClientResponseDelegate` implementation
might want to emit log messages to the `task`'s `logger`.

Modifications:

Changed the access level of `Task`'s `logger` property from `internal`
to `public`.

Result:

Users can access the `logger` property of a `Task` outside of the
`async-http-client`.
@dnadoba
Copy link
Collaborator

dnadoba commented Jan 22, 2023

@swift-server-bot test this please

@dnadoba dnadoba enabled auto-merge (squash) January 22, 2023 17:22
@dnadoba dnadoba merged commit 817d9aa into swift-server:main Jan 22, 2023
@dnadoba
Copy link
Collaborator

dnadoba commented Jan 22, 2023

@felixschlegel thanks a lot for the patch! sorry for the delay.

@felixschlegel
Copy link
Contributor Author

thanks a lot for the patch! sorry for the delay.

No problem 😄 Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task.Logger should be accessible to the delegate
7 participants