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

Updating bigint with FromStr trait for issue 81 #94

Closed
wants to merge 4 commits into from

Conversation

josediegorobles
Copy link
Contributor

This is my intent for issue 81
https://github.com/LNP-BP/rust-amplify/issues/81

Copy link
Member

@dr-orlovsky dr-orlovsky left a 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

@@ -16,6 +16,7 @@
// If not, see <https://opensource.org/licenses/MIT>.

use crate::error::ParseLengthError;
use std::convert::TryInto;
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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> {
Copy link
Member

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i'm seeing

@dr-orlovsky dr-orlovsky added C-num enhancement New feature or request labels Aug 3, 2021
@dr-orlovsky dr-orlovsky added this to the v3.7 milestone Aug 3, 2021
@dr-orlovsky
Copy link
Member

We need to fix CI and add tests here. Please let me know if you would like to proceed with this PR further

@josediegorobles
Copy link
Contributor Author

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
Copy link

codecov bot commented Aug 24, 2021

Codecov Report

Merging #94 (b51e06d) into master (f33cfae) will decrease coverage by 0.2%.
The diff coverage is 0.0%.

Impacted file tree graph

@@           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     
Impacted Files Coverage Δ
num/src/bigint.rs 85.0% <0.0%> (-1.4%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f33cfae...b51e06d. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement FromStr for big-sized num types
2 participants