Skip to content

Conversation

@CommanderStorm
Copy link
Contributor

Which issue does this PR close?

None, it is a cosmetic docs change.

Rationale for this change

Looking though the client docs I noticed that some parts could be more consistent and others could be more highlighted.
This PR improves the docs in this department.

What changes are included in this PR?

Changes like this:

image

Are there any user-facing changes?

🤷🏻‍♂️ It is only in the docs. Depends if you consider this user-facing. Likely yes. 😉

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Thanks! Looks mostly good to me

///
/// This is on by default, since http2 is known to be significantly slower than http1.
/// # See Also
/// * [`Self::with_http2_only`] if you only want to use http2
Copy link
Member

Choose a reason for hiding this comment

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

I think it would probably be good to have consistent casing of HTTP1 and HTTP2

Copy link
Contributor Author

@CommanderStorm CommanderStorm Nov 18, 2025

Choose a reason for hiding this comment

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

Changing the instances of the lowercased httpX is consistent with how we currently do doc comments.

Fixing the Spelling/punctuation is fine though. I fixed it in my last PR to HTTP/X which is used in the spec.

@alamb alamb changed the title docs(client): improve warnigngs and links in client doc comments docs(client): improve warnings and links in client doc comments Nov 20, 2025
@alamb
Copy link
Contributor

alamb commented Nov 20, 2025

Love it -- thank you @CommanderStorm and @kylebarron

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @CommanderStorm -- I think @kylebarron suggestions are spot on. I also left a few suggestions. Let us know what you think!

Copy link
Contributor Author

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Sorry for the long wait from my side

@alamb
Copy link
Contributor

alamb commented Nov 21, 2025

Sorry for the long wait from my side

No problems! Thank you for the contribution

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @CommanderStorm -- this looks like a great improvement to me

I am also interested to see what @kylebarron thinks too

@alamb
Copy link
Contributor

alamb commented Nov 21, 2025

I took the liberty of running cargo fmt and pushing to this branch to get the CI to pass cleanly

Copy link
Member

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@kylebarron kylebarron merged commit 0661843 into apache:main Nov 22, 2025
8 checks passed
@CommanderStorm CommanderStorm deleted the nicer-warnings branch November 22, 2025 04:23
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.

3 participants