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

docs: improve builder() method docs #76

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

EdJoPaTo
Copy link
Contributor

When I use an API I first care about the what, then how to achieve it. So reversing the statement feels better to me.

@@ -132,7 +132,7 @@ impl StructInputCtx {
};

let start_func_docs = format!(
"Use builder syntax to create an instance of [`{}`]",
"Create an instance of [`{}`] using builder syntax",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative:

Suggested change
"Create an instance of [`{}`] using builder syntax",
"Create an instance of [`{}`] with builder syntax",

Copy link
Contributor

@Veetaha Veetaha Aug 29, 2024

Choose a reason for hiding this comment

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

Hi! Thank you for opening a PR! I think this makes sense in this case.

Generally, I try to put the dynamic part of the message as the end. This way the static text around the interpolated value always stays at the beginning and "reading it is complete" by the time the person is reading the dynamic part.

A similar problem was in rust's panic messages before 1.73.0, where they changed it. In the older version of Rust you had to read the entire panic message first (which could take multiple lines actually), and only after that you'd read the line number (which is always a static limited size of text) at the end:

thread 'main' panicked at 'oh no! "ferris.txt" not found!', src/main.rs:3:5

Imagine reading "thread 'main' panicked at '{ huge debug formatted output by the end of which you forgot the start of the sentence }, src/main.rs:3:5'. It becomes harder to find that line reference and jump to it.

In this case there shouldn't be such a problem, because we interpolate a single identifier in the docs message, so I agree with this change, and I'll merge the PR once CI finishes.

I don't think it's critical to release this change immediately unless you think otherwise. I'd release it with some other fixes that I'm planning to do (e.g. cfg/cfg_attr conditional compilation support).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the case of the rust panic is different, as the panic message is usually quite long.

@EdJoPaTo EdJoPaTo changed the title docs: improve build() method docs docs: improve builder() method docs Aug 29, 2024
@Veetaha Veetaha merged commit d49a3b5 into elastio:master Aug 29, 2024
22 of 23 checks passed
@EdJoPaTo EdJoPaTo deleted the build-method-doc branch August 29, 2024 12:58
@github-actions github-actions bot mentioned this pull request Sep 1, 2024
This pull request was closed.
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.

2 participants