-
Notifications
You must be signed in to change notification settings - Fork 95
Implement float is_integer method #112
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
Conversation
In Go: 1.9.x, there is no math.Round method
|
we could drop support for |
please rebase on top of the latest |
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
+ Coverage 72.54% 72.61% +0.06%
==========================================
Files 60 60
Lines 11887 11895 +8
==========================================
+ Hits 8623 8637 +14
+ Misses 2739 2731 -8
- Partials 525 527 +2
Continue to review full report at Codecov.
|
py/float.go
Outdated
return nil, err | ||
} | ||
b := math.Abs(f - math.Round(f)) | ||
return NewBool(b < math.SmallestNonzeroFloat64), nil; |
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.
Is there any reason to use this logic?
CPython check the float is an integer by using the floor API.
o = (floor(x) == x) ? Py_True : Py_False;
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.
Calculating the difference between rounded value and original value.
If the difference is less than the minimum value of float64, I think that it would be an integer.
I'll check the Cpython and then fix that part by using floor API.
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.
py/float.go
Outdated
b := math.Abs(f - math.Round(f)) | ||
return NewBool(b < math.SmallestNonzeroFloat64), nil; | ||
} | ||
return nil, AttributeError |
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.
return nil, cantConvert(a, "float")
might be better
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'll change that part with cantConvert method.
py/float.go
Outdated
f, err := FloatAsFloat64(a) | ||
if err != nil { | ||
return nil, err | ||
f, _ := FloatAsFloat64(a) |
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.
Please return nil, err when FloatAsFloat64 return error
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'll change that.
py/float.go
Outdated
} | ||
return nil, AttributeError | ||
_, e := cantConvert(self, "float") |
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.
Please return cantConvert directly
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.
How do I use the second return value directly?
Sorry, I'm looking for that. But I can't find any information.
py/float.go
Outdated
} | ||
return False, nil | ||
} | ||
_, e := cantConvert(self, "float") |
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.
_, e := cantConvert(self, "float") | |
return cantConvert(self, "float") |
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.
hth :)
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.
Thanks for helping me out.
I couldn't think so....
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.
LGTM
ISSUE : #111