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

Inline http2 fixups #402

Merged
merged 4 commits into from Jul 3, 2023
Merged

Inline http2 fixups #402

merged 4 commits into from Jul 3, 2023

Conversation

rnro
Copy link
Contributor

@rnro rnro commented Jul 3, 2023

There are a few places where bugs / poorly structured code crept in during the inline multiplexer work. This attempts to address some of them:

  • overly long lines
  • mis-named variables
  • unnecessary use of generics in one case

@rnro rnro requested a review from glbrntt July 3, 2023 10:02
Comment on lines 308 to 310
private func _commonHTTPServerPipeline(configurator: @escaping (Channel) -> EventLoopFuture<Void>,
h2ConnectionChannelConfigurator: ((Channel) -> EventLoopFuture<Void>)?,
configureHTTP2Pipeline: @escaping (Channel) -> EventLoopFuture<T>) -> EventLoopFuture<Void> {
configureHTTP2Pipeline: @escaping (Channel) -> EventLoopFuture<Void>) -> EventLoopFuture<Void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: indentation is inconsistent now

@glbrntt glbrntt added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jul 3, 2023
@rnro rnro requested a review from glbrntt July 3, 2023 10:10
h2ConnectionChannelConfigurator: ((Channel) -> EventLoopFuture<Void>)?,
configureHTTP2Pipeline: @escaping (Channel) -> EventLoopFuture<Void>
) -> EventLoopFuture<Void> {
let h2ChannelConfigurator = { (channel: Channel) -> EventLoopFuture<Void> in
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't hate me but this line is now over-indented...

Suggested change
let h2ChannelConfigurator = { (channel: Channel) -> EventLoopFuture<Void> in
let h2ChannelConfigurator = { (channel: Channel) -> EventLoopFuture<Void> in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing minutia is what this is all about

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't hate me but that should be minutiae, minutia is singular.

Copy link
Contributor Author

@rnro rnro Jul 3, 2023

Choose a reason for hiding this comment

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

🤦 Getting that correct was my only criterion too

@rnro rnro requested a review from glbrntt July 3, 2023 10:31
rnro added 4 commits July 3, 2023 11:36
* remove redundant generics
* rename variables `clientHandler` -> `clientMultiplexer` where appropriate
@glbrntt glbrntt enabled auto-merge (squash) July 3, 2023 10:53
@glbrntt glbrntt merged commit b9fe8f5 into apple:main Jul 3, 2023
6 of 8 checks passed
@rnro rnro deleted the inline_HTTP2_fixups branch July 3, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants