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

Fix: new std feature to handle std error trait #43

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

h4sh3d
Copy link
Member

@h4sh3d h4sh3d commented Mar 13, 2023

Follow up to #39

no_std should be opt out. Before releasing 1.1.0 I didn't realize we would break compatibility with all dependent crates because std::error::Error implemented through thiserror was removed. core::error::Error is nightly for now so we can't make it work without this new std feature. For now I prefer using thiserror to handle derivation for us, it's common and 1 liner instead of manually keeping the implementation of Error::source.

Today I'm wondering if 1.1.1 is ok, but breaking dependents with: { version = "1", default-features = false } or if 2.0.0 is necessary. For the later I'd then remove check from default as this was a bad idea from the beginning.

@mangoplane would you check that we are equivalent in a no_std environment as in #39

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

Patch has no changes to coverable lines.

❗ Current head 2f927ed differs from pull request most recent head 0d37ed0. Consider uploading reports for the commit 0d37ed0 to get more accurate results

Files Changed Coverage
src/base58.rs ø

📢 Thoughts on this report? Let us know!.

@kayabaNerve
Copy link

Bah. I made #44 and only then saw this, sorry.

I'd vote to default feature flag std. With std, it relies on std::error::Error. When core::error::Error is stabilized, the std feature can be dropped.

@h4sh3d h4sh3d merged commit 0b09aa6 into monero-rs:main Sep 15, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants