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

Traits and impls for adding context and exit codes to results. #589

Merged
merged 5 commits into from
Sep 1, 2022

Conversation

anorth
Copy link
Member

@anorth anorth commented Aug 30, 2022

This is a backport from the decouple-fil+ branch. The new traits and conversion methods are extracted from @dignifiedquire's work in #214. We can have them now without necessarily following through all the work to convert everything over. I've converted a couple of the simpler actors to demonstrate, and we should use the new traits for new code.

@anorth anorth requested review from Stebalien and Kubuxu August 30, 2022 03:07
@anorth
Copy link
Member Author

anorth commented Aug 30, 2022

@Kubuxu you had questions about this in another PR. I believe these methods are still cheap because in implementation they still defer work through a closure. But with nice clean syntax. The with_ form should be used whenever a format!() is to be done.

@Kubuxu
Copy link
Contributor

Kubuxu commented Aug 30, 2022

100% agreed that the pattern is nicer, I just noticed that the closure pattern wasn't used in many places where format! was used.

@anorth anorth force-pushed the anorth/error-context branch 2 times, most recently from c7cabb0 to be262cc Compare August 31, 2022 22:02
@Stebalien
Copy link
Member

I have to say this is a lot nicer.

actors/multisig/src/lib.rs Outdated Show resolved Hide resolved
actors/multisig/src/lib.rs Outdated Show resolved Hide resolved
actors/multisig/src/lib.rs Outdated Show resolved Hide resolved
@Kubuxu
Copy link
Contributor

Kubuxu commented Sep 1, 2022

@Stebalien is there any simple way for us to get a lint for cases like ones I've pointed out above?
I think it is similar to this clippy lint: https://rust-lang.github.io/rust-clippy/master/#or_fun_call but I have no idea if we can somehow tack it onto our functions.


In 2min of searching I found this: https://github.com/trailofbits/dylint but introducing it would be quite a hassle just for this.

@Stebalien
Copy link
Member

is there any simple way for us to get a lint for cases like ones I've pointed out above?

I've never seen one, unfortunately.

@anorth anorth force-pushed the anorth/error-context branch from 847b390 to b835a0f Compare September 1, 2022 08:59
@anorth anorth requested a review from Kubuxu September 1, 2022 08:59
@codecov-commenter
Copy link

codecov-commenter commented Sep 1, 2022

Codecov Report

Merging #589 (847b390) into master (bacf044) will increase coverage by 0.55%.
The diff coverage is 81.54%.

❗ Current head 847b390 differs from pull request most recent head b835a0f. Consider uploading reports for the commit b835a0f to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
+ Coverage   83.92%   84.48%   +0.55%     
==========================================
  Files          88       88              
  Lines       18155    18081      -74     
==========================================
+ Hits        15237    15275      +38     
+ Misses       2918     2806     -112     
Impacted Files Coverage Δ
runtime/src/actor_error.rs 61.38% <58.82%> (-2.62%) ⬇️
actors/multisig/src/state.rs 94.79% <75.00%> (-4.05%) ⬇️
actors/multisig/src/lib.rs 95.23% <91.66%> (+15.50%) ⬆️
actors/init/src/lib.rs 96.77% <100.00%> (+1.38%) ⬆️
actors/init/src/state.rs 100.00% <100.00%> (ø)
actors/paych/src/lib.rs 92.71% <100.00%> (-0.15%) ⬇️
actors/system/src/lib.rs 94.23% <100.00%> (+1.63%) ⬆️
actors/verifreg/src/lib.rs 75.77% <100.00%> (+2.91%) ⬆️
runtime/src/builtin/shared.rs 92.30% <100.00%> (-1.45%) ⬇️
state/src/check.rs 87.23% <0.00%> (-0.43%) ⬇️
... and 7 more

@anorth anorth merged commit 2b446ef into master Sep 1, 2022
@anorth anorth deleted the anorth/error-context branch September 1, 2022 22:02
Stebalien pushed a commit that referenced this pull request Sep 20, 2022
Co-authored-by: dignifiedquire <me@dignifiedquire.com>
Stebalien pushed a commit that referenced this pull request Sep 20, 2022
Co-authored-by: dignifiedquire <me@dignifiedquire.com>
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Oct 3, 2022
shamb0 pushed a commit to shamb0/builtin-actors that referenced this pull request Jan 31, 2023
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.

4 participants