-
Notifications
You must be signed in to change notification settings - Fork 13
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
Updating bigint with FromStr trait for issue 81 #94
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls see my comments in the code
num/src/bigint.rs
Outdated
@@ -16,6 +16,7 @@ | |||
// If not, see <https://opensource.org/licenses/MIT>. | |||
|
|||
use crate::error::ParseLengthError; | |||
use std::convert::TryInto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to use core::
here to maintain no_std
compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but I see std in another partes of the code, getters for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out reading u64 Code but I'm not sure how to proceed.
If you can give me an indication I would be pleased
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getters are part of other crate, which is not no_std
one. The main indicator is when CI is broken, and here it indeed is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to figure out reading u64 Code but I'm not sure how to proceed.
not sure I understood your question
@@ -830,6 +831,18 @@ macro_rules! construct_bigint { | |||
} | |||
} | |||
|
|||
impl ::core::str::FromStr for $name { | |||
#[inline] | |||
fn from_str(s: &str) -> Result<Self, Self::Err> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand exactly what the code below does, so test case will be appreciated.
My understanding is that for FromStr implementation we support only decimals (like standard library) and should have from_str_radix
for other bases (which may be another PR, not necessary doing it here). Second, what you need is not a map over split iterator but a map over chunks
iterator...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, i'm seeing
We need to fix CI and add tests here. Please let me know if you would like to proceed with this PR further |
Ok, i will see tonight or tomorrow. I will continue if you think it's worth it |
Codecov Report
@@ Coverage Diff @@
## master rust-amplify/rust-amplify#94 +/- ##
========================================
- Coverage 66.6% 66.4% -0.2%
========================================
Files 30 30
Lines 3537 3545 +8
========================================
Hits 2355 2355
- Misses 1182 1190 +8
Continue to review full report at Codecov.
|
This is my intent for issue 81
https://github.com/LNP-BP/rust-amplify/issues/81