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

Improve discoverability with expect! as alias for unwrap! #872

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

dvdsk
Copy link
Contributor

@dvdsk dvdsk commented Oct 9, 2024

See related issue: #867 Its easy to assume unwrap! can not have a message like std's expect.

Adding an expect macro will help both autocomplete guide programmers looking for one and those who discount the unwrap entry in the defmt api docs based on their knowledge that std's unwrap can not have a message.

The documentation of the expect alias highlights that unwrap works with a message to and points to the unwrap macro docs for details.

See related issue: knurling-rs#867
Its easy to assume unwrap! can not have a message like std's expect.

Adding an expect macro will help both autocomplete guide programmers
looking for one and those who discount the unwrap entry in the defmt
api docs based on their knowledge that std's unwrap can not have a
message.

The documentation of the expect alias highlights that unwrap works with
a message to and points to the unwrap macro docs for details.
@dvdsk
Copy link
Contributor Author

dvdsk commented Oct 9, 2024

pretty sure CI breaking is unrelated since this PR is only 1 use line + some lines of comments

@dvdsk
Copy link
Contributor Author

dvdsk commented Oct 9, 2024

possible discussion point: because its an alias expect can be used without a message. Therefore you can not easily forbid unwrap's without a message by checking for the unwrap string.

@Urhengulas
Copy link
Member

pretty sure CI breaking is unrelated since this PR is only 1 use line + some lines of comments

That's true. Please wait for #871 which fixes the nightly tests.

I will mark this PR with "breaking change", which is not true, but the CI check compares against the latest published version and not against main and we have some breaking changes in the main branch already.

@Urhengulas Urhengulas added the breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version label Oct 11, 2024
/// ```
///
/// For the complete documentation see that of defmt's *unwrap* macro.
// note: Linking to unwrap is broken as of 2024-10-09, it links back to expect
Copy link
Member

Choose a reason for hiding this comment

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

What does that mean?

Copy link
Contributor Author

@dvdsk dvdsk Oct 11, 2024

Choose a reason for hiding this comment

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

ahh that means that link to unwrap written as: [unwrap](defmt::unwrap) does not go to the unwrap item but to the new expect introduced in this PR. That is why the word unwrap is italic and not a link in line 202.

@Urhengulas Urhengulas added this pull request to the merge queue Oct 11, 2024
Merged via the queue into knurling-rs:main with commit bfcb970 Oct 11, 2024
19 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change fix / feature / improvement involves a breaking change and needs to wait until next minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants