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

Allow any Displayable type for expect. #1968

Closed
wants to merge 1 commit into from

Conversation

clarfonthey
Copy link
Contributor

Formal RFC for #1922.

Rendered

@mark-i-m
Copy link
Member

I am not sure I understand how this saves on ergonomics... Wouldn't you still need to wrap the displayed value of the argument in text to get a coherent error message? Alternately, you could define Display yourself for some type, which seems to dwarf the overhead of simply writing expect(|e| panic!(...)) unless you are printing the same messages in many places.

Have I missed something?

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Apr 10, 2017
@aturon aturon self-assigned this Apr 25, 2017
@aturon
Copy link
Member

aturon commented Apr 29, 2017

Thanks for the RFC! However, I personally feel like the benefits here don't outweigh the added complexity of the API signature.

This is a case where method-style macros would be nice, to allow for something more akin to format!. But sadly method-style macros have numerous issues.

In any case, this feels quite niche to me, and I think the current ways of achieving the mentioned goals suffice, so I'm going to move to close.

@rfcbot fcp close

@rfcbot
Copy link
Collaborator

rfcbot commented Apr 29, 2017

Team member @aturon has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@aturon
Copy link
Member

aturon commented May 7, 2017

Ping @BurntSushi @Kimundi @brson @sfackler

@rfcbot
Copy link
Collaborator

rfcbot commented May 9, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added the final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. label May 9, 2017
@rfcbot
Copy link
Collaborator

rfcbot commented May 19, 2017

The final comment period is now complete.

@alexcrichton
Copy link
Member

Ok looks like not much more discussion happened during FCP, so I'm going to close this. Thanks again though for the RFC @clarcharr!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants