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

add expect_macro #3

Merged
merged 1 commit into from
Feb 8, 2018
Merged

add expect_macro #3

merged 1 commit into from
Feb 8, 2018

Conversation

vitiral
Copy link
Contributor

@vitiral vitiral commented Feb 8, 2018

This PR adds the expect! macro, which is an ergonomic replacement for unwrap()/expect(). The main advantages are:

  • Prints the line number/column of the failure with a better error message.
  • Can specify format parameters (The type of expect is expect(&str), so you have to use &format!(...))
  • Error conditions are lazily evaluated.

See the docs for expect_macro for more: http://docs.rs/expect_macro

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

bors r= @killercup

bors bot added a commit that referenced this pull request Feb 8, 2018
3: add expect_macro r=killercup a=vitiral

This PR adds the `expect!` macro, which is an ergonomic replacement for `unwrap()`/`expect()`. The main advantages are:

- Prints the line number/column of the failure with a better error message.
- Can specify format parameters (The type of `expect` is `expect(&str)`, so you have to use `&format!(...)`)
- Error conditions are lazily evaluated.
@vitiral vitiral requested a review from killercup February 8, 2018 16:14
@bors
Copy link

bors bot commented Feb 8, 2018

@bors bors bot merged commit bc293ec into master Feb 8, 2018
@killercup
Copy link

I don't think bors works like you think it works :) If you say r=foo it means "bors, foo reviewed this, it's good to merge"

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

well... you're right. I thought that was setting the reviewer ☹️

I really did want you to take a look at this before I merged it. If you don't agree/etc I can back it out.

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

Do I just assign a reviewer normally to assign a reviewer? I guess just mentioning you is probaly enough.

or is it

bors r? @killercup

@killercup
Copy link

Just use the github ui, bors has no review request command

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

okay, will do. Sorry I'm still getting used to this. I think I've seen r? on rust-lang/rust and figured it was standard 😄

What are your opinions of this macro?

@killercup
Copy link

Ah, r? in a rust-lang repo invokes @rust-highfive, not @bors!

Regarding the expect_macro crate, it looks good! Not sure if it will be wildly used, though: RFC 2091 will, once implemented (cf. rust-lang/rust#47809), make line numbers in backtraces way more useful, and I'm unsure if the added formatting power is enough to make up for the "it's no longer a method call" weirdness. So, I totally see its usefulness but I don't feel like the pain is enough to make a lot of people look for a not-built-in solution.

(I also just opened killercup/quicli#44 because I want to have quicli output useful back traces, but I this is for Result::Err. Panics are in general not pretty, though; and they probably shouldn't be. Maybe I'll use std::panic::set_hook to add a custom formatter to at least give more context.)

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

I was looking for that RFC, glad it was accepted!

Ya, I think once that RFC is accepted this will be largely overshadowned. However, that probably won't happen for several months.

Maybe that is reason enough to not include it in this library automatically. By the time this lib is stabilized, expect! will be outdated.

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

I think that is a good enough reason for me to not include it. I'll use it in my own projects, but there is no reason to add to the API when rust is about to just handle this.

@vitiral
Copy link
Contributor Author

vitiral commented Feb 8, 2018

Reverting this in #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants