Repository navigation
Suggest Ok(()) when encountering Result::<(), E>::Ok() - #44059
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
Looks nice. I left a few thoughts -- I think we could use to iterate once or twice on the exact message.
There was a problem hiding this comment.
this is a cute trick; is unit the terminology we are using here? somehow this message feels a bit "jargon-y" to me, but I'm not sure how to say it better.
cc @rust-lang/docs
There was a problem hiding this comment.
I confess, this message was also designed to trigger the 10 word limit for suggestions, otherwise you'd get "try creating a new unit value: ()", which really doesn't help the target audience.
I will reword it in a shorter manner and add a way to exempt this suggestion from being shortened to a label
There was a problem hiding this comment.
How about?
help: expected the unit value `()`; try to insert one more pair of parenthesis?
|
15 | let _: Result<(), String> = Ok(());
| ^^
There was a problem hiding this comment.
I like this one better, but the ; looks out of place.
maybe like this?
help: expected a unit value. Try to insert one more pair of parenthesis?
There was a problem hiding this comment.
I updated the message
There was a problem hiding this comment.
Nit: can you add a comment to this method and explain what some of the arguments are? Or perhaps just give this a clearer name, like suggest_unit_argument? (In that case, why not make it a bool?)
Alternatively, we could make this more general, but right now the added labels only make sense with ().
There was a problem hiding this comment.
Shouldn't this be suggesting as well?
There was a problem hiding this comment.
Yea. I haven't figured out yet what's going on...
aidanhs
added
the
S-waiting-on-author
|
Seems good to me now. @bors r+ |
|
📌 Commit 0b72497 has been approved by |
| sugg_span.hi = sugg_span.lo; | ||
| err.span_suggestion( | ||
| sugg_span, | ||
| "expected the unit value `()`. You can create one with a pair of parenthesis", |
There was a problem hiding this comment.
pair of parentheses, plural
Suggest `Ok(())` when encountering `Result::<(), E>::Ok()`
|
☀️ Test successful - status-appveyor, status-travis |
No description provided.