Skip to content

Conversation

@tdelabro
Copy link
Contributor

Remove dep to thiserror

Description

Rather than alternating between thiserror and thiserror-no-std depending on the feature "std", we can use thiserror-no-std/std.

Checklist

  • CHANGELOG has been updated.

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #1279 (36a73d2) into main (b7725bb) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1279   +/-   ##
=======================================
  Coverage   97.56%   97.56%           
=======================================
  Files          90       90           
  Lines       36260    36260           
=======================================
  Hits        35377    35377           
  Misses        883      883           
Impacted Files Coverage Δ
vm/src/cairo_run.rs 99.50% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@fmoletta fmoletta left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

🚀 🚀

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @tdelabro!

IMO it's ready to merge, but please move the changelog entry up as @MegaRedHand pointed out. If you have time, could you also check my question in the other comment? Not a blocker, it's mostly curiosity.

"parse-hyperlinks/std",
"felt/std",
"dep:num-prime",
"thiserror-no-std/std",
Copy link
Contributor

Choose a reason for hiding this comment

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

All of this makes me wonder, do we actually use anything that requires the std feature on thiserror? Did you try building without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

The std version of thiserror generate the following line (which is not generated in no_std):

impl std::error::Error for <MyErrorType> {}

Copy link
Contributor Author

@tdelabro tdelabro Jun 28, 2023

Choose a reason for hiding this comment

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

In the future rust will move the Error trait in core tho

@tdelabro tdelabro force-pushed the remove-thiserror-dep branch from 3e350ef to 36a73d2 Compare June 28, 2023 08:31
@MegaRedHand MegaRedHand added this pull request to the merge queue Jun 28, 2023
Merged via the queue into lambdaclass:main with commit fde54b4 Jun 28, 2023
@tdelabro tdelabro deleted the remove-thiserror-dep branch June 30, 2023 09:40
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 4, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 4, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 25, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jul 25, 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