Skip to content

Conversation

@zvavybir
Copy link
Contributor

@zvavybir zvavybir commented Oct 10, 2024

The current wording (e.g. use of `ok_or` followed by a function call) is potentially confusing (at least it confused me) by suggesting that the function that follows the (in this case) ok_or is the problem and not the function that is an argument to it.

The code in my program that triggered the confusing message is the following:

let file_id = buf
    .lines()
    .next()
    .ok_or((
        InternalError::ProblemReadingFromInbox,
        anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
    ))
    .html_context(stream, lang)?;

I thought that html_context was the problem and that I should do something along the following lines:

let file_id = buf
    .lines()
    .next()
    .ok_or_else(
        (
            InternalError::ProblemReadingFromInbox,
            anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
        ),
        html_context(stream, lang),
    )?

This is of course wrong. My confusion was only cleared up through the help message indicating what I should try instead.

If someone has a better idea of a replacement wording (currently e.g. function call inside of `ok_or`), I'm all ears.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 10, 2024
@Alexendoo
Copy link
Member

Alexendoo commented Oct 11, 2024

Yeah that's better, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 8c46c49 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 8c46c49 with merge 29a0616...

bors added a commit that referenced this pull request Oct 11, 2024
Improved wording of or_fun_call lint

The current wording (e.g. ``use of `ok_or` followed by a function call``) is potentially confusing (at least it confused me) by suggesting that the function that follows the (in this case) `ok_or` is the problem and not the function that is an argument to it.

The code in my program that triggered the confusing message is the following:
```rust
let file_id = buf
    .lines()
    .next()
    .ok_or((
        InternalError::ProblemReadingFromInbox,
        anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
    ))
    .html_context(stream, lang)?;
```
I thought that `html_context` was the problem and that I should do something along the following lines:
```rust
let file_id = buf
    .lines()
    .next()
    .ok_or_else(
        (
            InternalError::ProblemReadingFromInbox,
            anyhow!("No first line in inbox response ({file:?}): {buf:?}"),
        ),
        html_context(stream, lang),
    )?
```
This is of course wrong.  My confusion was only cleared up through the help message indicating what I should try instead.

If someone has a better idea of a replacement wording (currently e.g. ``` function call inside of `ok_or` ```), I'm all ears.

changelog [`or_fun_call`]: Improved confusing wording
@bors
Copy link
Contributor

bors commented Oct 11, 2024

💔 Test failed - checks-action_test

@Alexendoo
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 8c46c49 with merge b85f632...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing b85f632 to master...

@bors bors merged commit b85f632 into rust-lang:master Oct 11, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants