-
Notifications
You must be signed in to change notification settings - Fork 596
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
Added fmt for signed int. #5308
Conversation
19fd76f
to
e5838a8
Compare
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.
Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
corelib/src/fmt.cairo
line 38 at r1 (raw file):
> of Display<Signed> { fn fmt(self: @Signed, ref f: Formatter) -> Result<(), Error> { let (abs, sign) = (*self).abs_and_sign();
doesn't .
desnap?
Suggestion:
self.abs_and_sign()
corelib/src/fmt.cairo
line 42 at r1 (raw file):
write!(f, "-")?; } Display::fmt(@abs, ref f)
Suggestion:
(@abs).fmt(ref f)
corelib/src/fmt.cairo
line 94 at r1 (raw file):
> of Debug<Signed> { fn fmt(self: @Signed, ref f: Formatter) -> Result<(), Error> { Display::fmt(self, ref f)
Suggestion:
self.fmt(ref f)
corelib/src/integer.cairo
line 3048 at r1 (raw file):
pub(crate) trait AbsAndSign<Signed, Unsigned> { /// Returns the absolute value of the `Signed` value as `Unsigned` and the original sign. fn abs_and_sign(self: Signed) -> (Unsigned, bool);
Just to make it super explicit - say what sign is true and what sign is false.
corelib/src/test/fmt_test.cairo
line 17 at r1 (raw file):
assert(format!("{}", nz_value) == "1", 'non zero bad formatting'); assert( format!("{}_{}_{}_{}_{}", 1_i8, 2_i16, 3_i32, 4_i64, 5_i128) == "1_2_3_4_5",
please add 0
e5838a8
to
1cc7178
Compare
commit-id:2285c146
1cc7178
to
1ad3678
Compare
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.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @yuvalsw)
corelib/src/fmt.cairo
line 38 at r1 (raw file):
Previously, yuvalsw wrote…
doesn't
.
desnap?
it only snaps - it doesn't desnap.
corelib/src/fmt.cairo
line 94 at r1 (raw file):
> of Debug<Signed> { fn fmt(self: @Signed, ref f: Formatter) -> Result<(), Error> { Display::fmt(self, ref f)
can't - needs to specifically call the correct fmt.
corelib/src/integer.cairo
line 3048 at r1 (raw file):
Previously, yuvalsw wrote…
Just to make it super explicit - say what sign is true and what sign is false.
Done.
corelib/src/test/fmt_test.cairo
line 17 at r1 (raw file):
Previously, yuvalsw wrote…
please add 0
Done.
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.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)
corelib/src/fmt.cairo
line 94 at r1 (raw file):
Previously, orizi wrote…
can't - needs to specifically call the correct fmt.
what other formats do you have in context?
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @yuvalsw)
corelib/src/fmt.cairo
line 94 at r1 (raw file):
Previously, yuvalsw wrote…
what other formats do you have in context?
The Display vs the Debug
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)
corelib/src/fmt.cairo
line 94 at r1 (raw file):
Previously, orizi wrote…
The Display vs the Debug
Does it find the Debug that you are currently in (that is, DebugSignedInteger)? If so, it's out of context, but needs to be resolved.
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.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @yuvalsw)
corelib/src/fmt.cairo
line 94 at r1 (raw file):
Previously, yuvalsw wrote…
Does it find the Debug that you are currently in (that is, DebugSignedInteger)? If so, it's out of context, but needs to be resolved.
it finds it - and it is in context.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)
Closes issue #5298 .
This change is