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.

Sign up for GitHub

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

Jump to bottom

Perhaps you meant: foo, bar or foobar #6550

Merged
merged 1 commit into from
Jan 20, 2019
Merged

Perhaps you meant: foo, bar or foobar #6550

merged 1 commit into from
Jan 20, 2019

Conversation

In-line
Copy link
Contributor

@In-line In-line commented Jan 14, 2019 •
edited
Loading

Hi! Rust project is very cool, but I noticed some minor issues. In every place did you mean: bla bla end with the question mark, so I decided to include it here too.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dwijnand (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@In-line In-line force-pushed the did-you-mean-with-question-mark branch from 7d5e006 to 2142b41 Compare January 14, 2019 19:37
@In-line In-line changed the title Fix typo and add qeuestion mark in the necessary places Fix typo and add question mark in the necessary places Jan 14, 2019
alexcrichton
alexcrichton reviewed Jan 14, 2019
View reviewed changes
tests/testsuite/directory.rs Outdated
@@ -189,7 +189,7 @@ error: failed to compile `bar v0.1.0`, intermediate artifacts can be found at `[
Caused by:
no matching package named `baz` found
location searched: registry `https://github.com/rust-lang/crates.io-index`
did you mean: bar, foo
did you mean: bar, foo?
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the ? would look better here before the colon? I'm a little worried about it accidentally looking like it's part of the crate name

Copy link
Contributor Author

@In-line In-line Jan 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking the same.

  • did you mean: bar, foo ?
  • did you mean? bar, foo
  • Perhaps you mean: bar, foo

Maybe variant without any questions is the best?

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me! although it may want to be worded "perhaps you meant: foo, bar"

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, perhaps you meant: foo, bar looks good.

Alternatively we can quote them: did you mean: "bar", "foo"?.

And, to be honest, I think we could also throw an "or" in there, so with 3 cases it would look like:

  • perhaps you meant: bar, foo, or baz, or
  • did you mean: "bar", "foo", or "baz"?

(Oxford comma usage optional! 😉)

But I'd be happy to accept either of the first two.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of "or", although I'd prefer to avoid using quotes as it makes it easier to copy/paste without them sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcrichton @dwijnand What is your decision mighty Dictators?

Copy link
Member

Choose a reason for hiding this comment

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

Go with perhaps you meant: bar, foo, baz.
If you're feeling ambitious: perhaps you meant: bar, foo, or baz 😄

@In-line In-line force-pushed the did-you-mean-with-question-mark branch from 2142b41 to a8a9169 Compare January 14, 2019 22:02
@In-line In-line changed the title Fix typo and add question mark in the necessary places Add question mark in the necessary places Jan 14, 2019
@In-line
Copy link
Contributor Author

In-line commented Jan 14, 2019

I splitted this PR to two, so typo fix can be merged sooner :)
#6552

@In-line In-line force-pushed the did-you-mean-with-question-mark branch from a8a9169 to 74176ec Compare January 19, 2019 13:28
@In-line In-line changed the title Add question mark in the necessary places Perhaps you meant: foo, bar or foobar Jan 19, 2019
In-line
In-line commented Jan 19, 2019
View reviewed changes
src/cargo/core/resolver/errors.rs
|acc, (i, el)| match i {
0 => acc + el,
i if names.len() - 1 == i && candidates.len() <= 3 => acc + " or " + el,
_ => acc + ", " + el,
Copy link
Contributor Author

@In-line In-line Jan 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Is this code idiomatic way to achieve what we want?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but it looks like how I'd do it :)

@In-line In-line force-pushed the did-you-mean-with-question-mark branch from 74176ec to 67dbfe5 Compare January 19, 2019 13:34
@In-line In-line changed the title Perhaps you meant: foo, bar or foobar Perhaps you meant: foo, bar or foobar Jan 19, 2019
@dwijnand
Copy link
Member

Excellent, thank you!

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 20, 2019

📌 Commit 67dbfe5 has been approved by dwijnand

@bors
Copy link
Contributor

bors commented Jan 20, 2019

⌛ Testing commit 67dbfe5 with merge 6d5cd59...

bors added a commit that referenced this pull request Jan 20, 2019
@bors
Auto merge of #6550 - In-line:did-you-mean-with-question-mark, r=dwij…
6d5cd59 
…nand

Perhaps you meant: foo, bar or foobar

Hi! Rust project is very cool, but I noticed some minor issues. In every place `did you mean: bla bla` end with the question mark, so I decided to include it here too.
@bors
Copy link
Contributor

bors commented Jan 20, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: dwijnand
Pushing 6d5cd59 to master...

@bors bors merged commit 67dbfe5 into rust-lang:master Jan 20, 2019
@ehuss ehuss mentioned this pull request Jan 20, 2019
Merged
bors added a commit to rust-lang/rust that referenced this pull request Jan 21, 2019
@bors
Auto merge of #57789 - ehuss:update-cargo, r=Mark-Simulacrum
7bac68a 
Update cargo

Pull in fix for #57774.

6 commits in ffe65875fd05018599ad07e7389e99050c7915be..907c0febe7045fa02dff2a35c5e36d3bd59ea50d
2019-01-17 23:57:50 +0000 to 2019-01-20 22:31:07 +0000
- Put mtime-on-use behind a feature flag. (rust-lang/cargo#6573)
- Fix a typo in the unstable docs (rust-lang/cargo#6569)
- Perhaps you meant: foo, bar or foobar (rust-lang/cargo#6550)
- Refactor: Create uninstall submodule (rust-lang/cargo#6557)
- Fix spurious Windows errors with switch_features_rerun. (rust-lang/cargo#6561)
- Stop building on master on Travis. (rust-lang/cargo#6562)

r? @Mark-Simulacrum
@In-line In-line deleted the did-you-mean-with-question-mark branch January 23, 2019 12:26
@ehuss ehuss added this to the 1.34.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexcrichton alexcrichton alexcrichton left review comments

@dwijnand dwijnand dwijnand left review comments

Assignees

@dwijnand dwijnand

Labels
None yet
Projects
None yet
Milestone
1.34.0
Development

Successfully merging this pull request may close these issues.
6 participants
@In-line @rust-highfive @dwijnand @bors @alexcrichton @ehuss