Skip to content
This repository has been archived by the owner on Mar 29, 2022. It is now read-only.

Re-add Option and Result. #68

Merged
merged 17 commits into from
Mar 26, 2022
Merged

Re-add Option and Result. #68

merged 17 commits into from
Mar 26, 2022

Conversation

adlerjohn
Copy link
Contributor

@adlerjohn adlerjohn commented Mar 10, 2022

Reverts #63, now that FuelLabs/sway#815 is in.

@adlerjohn adlerjohn self-assigned this Mar 10, 2022
@nfurfaro
Copy link
Contributor

@adlerjohn Are there any blockers for this, or can we wrap it up and merge soonish?

@adlerjohn
Copy link
Contributor Author

We have to wait for v0.7.0 of forc, which can be cut after the above PR is merged.

@nfurfaro
Copy link
Contributor

Ah, I see, I took "now that FuelLabs/sway#815 is in." to mean 815 was done.

@nfurfaro
Copy link
Contributor

@adlerjohn can we move ahead on this with the release of Forc v0.7.0 ?

@adlerjohn adlerjohn marked this pull request as ready for review March 22, 2022 20:45
@adlerjohn adlerjohn requested review from nfurfaro, otrho and sezna March 22, 2022 20:45
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but we should ensure this is tested before merging.
Can I push some tests to your branch here @adlerjohn ?

otrho
otrho previously approved these changes Mar 22, 2022
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tiny bit strange that is_none() and is_err() are matching against Some and Ok and returning false, instead of matching None and Err, but this works.

@adlerjohn adlerjohn marked this pull request as draft March 22, 2022 22:30
@adlerjohn
Copy link
Contributor Author

It isn't possible to instantiate a Result, so impossible to test. Still waiting on FuelLabs/sway#814 after all!

@adlerjohn
Copy link
Contributor Author

These tests still need to be implemented:

fn test_ok() {
// let r = Result::<u64, ()>::Ok(42u64);
// if ( !result_is_ok(r) || result_is_err(r)) {
// panic(0);
// }
}
fn test_err() {
// let r = Result::<(), ()>::Err(());
// if (result_is_ok(r) || !result_is_err(r)) {
// panic(0);
// }
}

@adlerjohn adlerjohn marked this pull request as ready for review March 26, 2022 00:29
@adlerjohn adlerjohn requested review from nfurfaro and otrho March 26, 2022 00:29
otrho
otrho previously approved these changes Mar 26, 2022
Copy link
Contributor

@otrho otrho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So Emily's recent changes fixed the problems with instantiation?

@adlerjohn
Copy link
Contributor Author

So Emily's recent changes fixed the problems with instantiation?
yes

src/lib.sw Outdated Show resolved Hide resolved
sezna
sezna previously approved these changes Mar 26, 2022
@adlerjohn adlerjohn dismissed stale reviews from sezna and otrho via 147de3d March 26, 2022 00:45
@adlerjohn adlerjohn requested a review from nfurfaro March 26, 2022 00:46
Copy link
Contributor

@nfurfaro nfurfaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

@adlerjohn adlerjohn merged commit bb4565a into master Mar 26, 2022
@adlerjohn adlerjohn deleted the adlerjohn/option_result branch March 26, 2022 00:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants