Avoid unwrap in examples

[bjoernQ]

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

• I have updated existing examples or added new ones (if applicable).
• I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
• My changes were added to the CHANGELOG.md in the proper section.
• I have added necessary changes to user code to the Migration Guide.
• My changes are in accordance to the esp-rs API guidelines

Extra:

• I have read the CONTRIBUTING.md guide and followed its instructions.

Pull Request Details 📖

Description

Remove the C-QUESTION-MARK exception from API_GUIDELINES.md and get rid of unwraps in doc-examples.

Testing

Generate docs and have a look

[bugadani]

I thought we said not to do this, because user code can't really use ? by default.

[bjoernQ]

I thought we said not to do this, because user code can't really use ? by default.

Was that the reason? I thought we wanted to avoid the effort to do it - in any case teaching users to always unwrap isn't a good thing

[bugadani]

Was that the reason?

Well I'm just repeating myself because nobody else really stated an opinion on #2811 (except Kirill, but his opinion may have been somewhat derailed by mine).

The users can't really do anything else, but handle the error or unwrap (or ignore). They'll copy the ? code and open issues that the documented examples don't work.

I agree with some parts of the PR, where you handle errors instead of unwraps, but I think we shouldn't just search-and-replace unwraps with ?.

[bjoernQ]

Maybe I missed that discussion - I don't care much about this PR so whatever

[MabezDev]

I think we should change our stance here, and accept this PR.

Whilst ? into main probably isn't that feasible for no_std users, the can still use the ? operator in library code. Plus they can just trampoline to main2() -> Result<(), /* Type */> where type could be a giga enum of all errors or Box<dyn Error> and handle the error right at the top.

Overall the documentation looks cleaner too, unwraps are quite noisy.

[MabezDev]

Suggested change

	//! writeln!(serial, "Hello world!").ok();
	//! writeln!(serial, "Hello world!")?;

[MabezDev]

There a few of these in this file, could you resolve them all?

[bugadani]

I think I will remain in opposition still, ? is a bit too easy to miss. Also, the people who are able to write the scaffolding to support ?, they already know what to do about unwraps. Less experienced people will just be confused why we document something that doesn't work the way we wrote the example.

[MabezDev]

I think I will remain in opposition still, ? is a bit too easy to miss

Not sure I follow this? What's there to miss 😅? They'll have to handle the error some how, and the compiler will complain at them if they don't. Plus the docs display the signature.

Also, the people who are able to write the scaffolding to support ?, they already know what to do about unwraps. Less experienced people will just be confused why we document something that doesn't work the way we wrote the example.

I do some what agree with this, but ? is used everywhere in documentation, and in real code all across the ecosystem. What we document here can, and will, work outside of main, and there is a work around for main with a trampoline function.

I'm still not convinced we shouldn't accept this PR.

[bjoernQ]

If we want to build APIs for users who don't understand Rust's error handling, I guess we need to change a bit more

[Dominaezzz]

.unwrap() is more copy and paste friendly. When I copy examples with ?, I always have to replace them with unwrap() or .map_error(|e| ...)?.

the can still use the ? operator in library code

This is especially true in library code. If I'm at the point in development where I'm copying examples from the Rust doc, ? will almost certainly fail to compile as I don't know about the errors yet. Having everything as .unwrap() to begin with means I can get to what I'm doing quicker and deal with the error handling later.

Not sure I follow this? What's there to miss 😅? They'll have to handle the error some how, and the compiler will complain at them if they don't. Plus the docs display the signature.

I agree with Daniel that it's hard to see, more round trips with compiler. At least with .unwrap() the compiler will leave me alone and I can deal with the error handling later, which in robust firmwares, isn't just as simple as doing ?, as you typically want to inspect the contents of the error and handle some cases.

For code snippets in Rust modules .unwrap() is easier to consume. (I do agree that it's a bit noisy)
For the full/proper examples in the examples/ folder, ? could work.

[bjoernQ]

If I'm at the point in development where I'm copying examples from the Rust doc, ? will almost certainly fail to compile as I don't know about the errors yet. Having everything as .unwrap() to begin with means I can get to what I'm doing quicker and deal with the error handling later.

I'd say someone at that level will copy s.th. and never think about error handling if everything is "fine". If they run into compiler errors, they at least need to think about it

Additionally, the more involved examples (assuming no one copies a single function call) don't show all the code (because it's distracting noise) and copying them won't get you with something working no matter if the code unwraps or not.

In any case we are making a lot of assumption what users do and don't do, what they know and don't know. Luckily, this (in contrast of the actual API) is something we can revisit any time. Changing all that back shouldn't be an issue if we see users are running into problems (I don't say they won't - we just don't know currently)

Would be really interesting how an average (real) user journey looks like. We are assuming they read "The Book", setup things and verify everything is working fine by running "Hello World" .... experience suggest s.th. different at least for some users 😆 ...

[MabezDev]

LGTM, thanks for addressing my concerns.

Imo this is an improvement, but I do understand the push back (and thanks for discussing it). Like @bjoernQ this is docs and we can change them at any time if we feel this is too confusing as a whole.

[bugadani]

@bjoernQ sorry this ran up against the guideline rename, and now has a merge conflict. Can you please add 5896bb5 when you are rebasing this PR?