search

jeremyletang / rgtk

GTK+ bindings and wrappers for Rust (DEPRECATED SEE https://github.com/rust-gnome )
GNU Lesser General Public License v3.0
121 stars 22 forks source link

Replace match...Err block with equivalent unwrap() #238

Closed frewsxcv closed 9 years ago

GuillaumeGomez commented 9 years ago

Thanks for that ! I wait travis to confirm and I merge.

frewsxcv commented 9 years ago

Oh hold on, i'll fix that

GuillaumeGomez commented 9 years ago

Now that's good. Thanks for your work !

gkoz commented 9 years ago

Last I checked it wasn't actually equivalent, because the second part of result is a String instead of an Error. Look at the output in the failure mode.

frewsxcv commented 9 years ago

@gkoz I don't understand, can you elaborate your concerns?

gkoz commented 9 years ago

Make pkg-config fail and see what cargo build prints out.

frewsxcv commented 9 years ago

Make pkg-config fail

What do you mean by this?

gkoz commented 9 years ago

The match you replaced had two arms. Have you tested both scenarios after making the change and confirmed that in either of them the new code behaves in exactly the same way [as before]?

frewsxcv commented 9 years ago

It should functionally be the same, but it looks like it also adds extra text when it panics

https://github.com/rust-lang/rust/blob/ecf8c64e1b1b60f228f0c472c0b0dab4a5b5aa61/src/libcore/result.rs#L781

Feel free to revert my commit if you don't want that text to be shown

gkoz commented 9 years ago

Note the {:?} there? The match used {} and it makes a difference in this case, because debug formatting of String isn't very readable. It becomes fairly obvious when you test the failure mode of the build script.

GuillaumeGomez commented 9 years ago

Sorry @frewsxcv but I have to revert your PR. Thanks @gkoz.

frewsxcv commented 9 years ago

All good, sorry for the confusion. Keep up the great work with this project

GuillaumeGomez commented 9 years ago

@frewsxcv No problem. Don't hesitate to add anymore stuff, this project is far to be finished and any help is very appreciated !

gkoz commented 9 years ago

I've discussed this issue on IRC and the sentiment is that unwrap should only be used when it's not expected to panic therefore the end users aren't supposed to see the output it produces. If we, like in this case, want to panic with a message, better do it explicitly.