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

Split up HBRequestContext from HBRequest #240

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

Joannis
Copy link
Contributor

@Joannis Joannis commented Oct 2, 2023

No description provided.

@Joannis Joannis changed the base branch from main to 2.x.x October 2, 2023 11:43
@tib
Copy link
Contributor

tib commented Oct 2, 2023

Can't wait to try this out @Joannis 👍

Copy link
Member

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

Looks good in general. Couple of question surrounding why a couple of things are still in HBRequest and not the HBRequestContext.


/// Request ID (Uses ID attached to loggger to avoid replication)
public var id: String { self.logger[metadataKey: "hb_id"]!.description }
public let id: String
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we can move to context as well? I was kind of hoping we could ditch HBRequest for HBHTTPRequest. The ID was something you asked for initially what was the intention for it.

Copy link
Member

Choose a reason for hiding this comment

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

In fact the id is stored in both request and context at the moment

self.uri = uri
self.version = version
self.method = method
self.headers = headers
self.applicationContext = applicationContext
self.context = context
self.endpointPath = .init(endpointPath)
Copy link
Member

Choose a reason for hiding this comment

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

Similar to id any reason we can't move endpointPath to the context as well

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

Attention: 28 lines in your changes are missing coverage. Please review.

Files Coverage Δ
...ummingbird/AsyncAwaitSupport/AsyncMiddleware.swift 85.71% <100.00%> (ø)
...Hummingbird/AsyncAwaitSupport/AsyncResponder.swift 100.00% <100.00%> (ø)
...ingbird/AsyncAwaitSupport/RouteHandler+async.swift 100.00% <100.00%> (ø)
Sources/Hummingbird/Codable/RequestDecodable.swift 100.00% <100.00%> (ø)
...ources/Hummingbird/Codable/ResponseEncodable.swift 100.00% <100.00%> (ø)
...ources/Hummingbird/Middleware/CORSMiddleware.swift 94.91% <100.00%> (ø)
...ces/Hummingbird/Middleware/MetricsMiddleware.swift 100.00% <100.00%> (ø)
Sources/Hummingbird/Middleware/Middleware.swift 100.00% <ø> (ø)
...ces/Hummingbird/Middleware/TracingMiddleware.swift 86.27% <100.00%> (ø)
Sources/Hummingbird/Router/Router.swift 100.00% <100.00%> (ø)
... and 16 more

📢 Thoughts on this report? Let us know!.

Copy link
Member

@adam-fowler adam-fowler left a comment

Choose a reason for hiding this comment

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

LGTM

@adam-fowler adam-fowler merged commit 80dc45e into 2.x.x Oct 4, 2023
3 of 5 checks passed
@adam-fowler adam-fowler deleted the feature/split-up-hbrequest branch October 4, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants