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

Use ? instead of unwrap() #870

Merged
merged 1 commit into from Aug 7, 2018

Conversation

Projects
None yet
6 participants
@kzys
Contributor

kzys commented Aug 6, 2018

This change addresses #833, while there are still some unwrap() though.

@mvines mvines added the CI label Aug 6, 2018

@solana-grimes solana-grimes removed the CI label Aug 6, 2018

@kzys

This comment has been minimized.

Contributor

kzys commented Aug 6, 2018

@mvines

This comment has been minimized.

Member

mvines commented Aug 6, 2018

@kzys thanks, yeah it's not this PR. Sorry for the speed bump. #871 covers updating those machines

@@ -71,7 +71,7 @@ where
}
/// ring a -> b -> c -> d -> e -> a
#[test]
fn gossip_ring() {
fn gossip_ring() -> Result<(), solana::result::Error> {

This comment has been minimized.

@rob-solana

rob-solana Aug 6, 2018

Contributor

where do we define solana::result::Error? just curious, might be useful for #822

This comment has been minimized.

@kzys

kzys Aug 6, 2018

Contributor

src/result.rs is already having this Error :)

This comment has been minimized.

@garious

garious Aug 6, 2018

Member

You can tighten up that code with:

use solana::result::Result;
fn gossip_ring() -> Result<()> {
@garious

This comment has been minimized.

Member

garious commented Aug 6, 2018

@aeyakovenko, what do you think about this? My feeling is that when you're using either unwrap() or ?, it's part of the test that you're not intending to test, and should almost always succeed. When it doesn't succeed, debugging it with RUST_BACKTRACE=1 is the normal dev process, and where you'd get that line number. Therefore, I'm leaning toward preferring the use of ? over unwrap(), as it makes the unit tests more readable. For me, readability is a higher priority than non-RUST_BACKTRACE debuggability. Thoughts?

@garious

This comment has been minimized.

Member

garious commented Aug 6, 2018

@kzys, looks like we'll need to you rebase on the latest multinode.rs. Thanks for taking this on!

@aeyakovenko

This comment has been minimized.

Member

aeyakovenko commented Aug 6, 2018

how do you see RUST_BACKTRACE=1 helping? ? wont panic and you will go up the stack out of the test.

@kzys

This comment has been minimized.

Contributor

kzys commented Aug 6, 2018

You can see the actual backtrace on #833

I can't recall that the line number on the stacktrace points ? or the function itself. I can take a look that tonight.

@@ -164,7 +166,7 @@ pub fn crdt_retransmit() {
assert!(done);
let mut b = Blob::default();
b.meta.size = 10;
Crdt::retransmit(&c1, &Arc::new(RwLock::new(b)), &tn1).unwrap();
Crdt::retransmit(&c1, &Arc::new(RwLock::new(b)), &tn1)?;

This comment has been minimized.

@aeyakovenko

aeyakovenko Aug 6, 2018

Member

if this function fails, you wont see it in the stackframe. unless the Err itself is generated with a line number.

Use ? instead of unwrap()
This change addresses #833, while there are still some unwrap() though.

@kzys kzys force-pushed the kzys:reduce-unwrap branch from 0676149 to c4e6a83 Aug 7, 2018

@kzys

This comment has been minimized.

Contributor

kzys commented Aug 7, 2018

I've updated the pull request to rebase and use solana::result::Result<>

@mvines mvines added the CI label Aug 7, 2018

@solana-grimes solana-grimes removed the CI label Aug 7, 2018

@garious garious added the CI label Aug 7, 2018

@solana-grimes solana-grimes removed the CI label Aug 7, 2018

@garious

garious approved these changes Aug 7, 2018

@aeyakovenko, I prefer this notation for a few reasons:

  • More readable
  • Debugging those Errors might encourage us to create more descriptive Error types.
  • Test code could be copied into prod code and vice versa. It'll be easier to create tests from existing code and easier to bootstrap code from samples in test.

A strong +1 from me.

@garious

This comment has been minimized.

Member

garious commented Aug 7, 2018

@mvines, what's up with Grimes?

@mvines

This comment has been minimized.

Member

mvines commented Aug 7, 2018

@mvines, what's up with Grimes?

what do you mean?

@garious garious requested a review from mvines Aug 7, 2018

@garious

This comment has been minimized.

Member

garious commented Aug 7, 2018

@mvines, I mean that each time we tried to add the CI label, Grimes removed it.

@mvines

This comment has been minimized.

Member

mvines commented Aug 7, 2018

Ah so that's by design. The ci label is only good for the particular commit it was added to, so it's removed immediately after ci is started for that commit. We can certainly discuss if that's the right design or not on discord though!

@mvines

mvines approved these changes Aug 7, 2018

@garious garious added the automerge label Aug 7, 2018

@solana-grimes solana-grimes merged commit 9c1b628 into solana-labs:master Aug 7, 2018

2 checks passed

buildkite/solana Build #2152 passed (13 hours, 48 minutes, 41 seconds)
Details
ci-gate Pull Request accepted for CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment