cue icon indicating copy to clipboard operation
cue copied to clipboard

cue: float64 support for zero

Open kcburge opened this issue 2 years ago • 4 comments

Fix the error when attempting to use strconv.FormatFloat with a zero value:

cannot use 0.0 (type float) as float64 in argument 0 to strconv.FormatFloat: value was rounded down

The tests for maximum and minimum in Float64() do not allow for zero. The below check incorrectly sees zero as smaller, because the Sign() of zero (0) is less than the Sign() of smallestPosFloat64 (1). So, it was clear this logic was not accounting for zero.

	if n.X.Cmp(smallestPosFloat64) == -1 {

closes #1669

Signed-off-by: Kevin Burge [email protected]

kcburge avatar Apr 26 '22 23:04 kcburge

Good catch.

mpvl avatar Apr 27 '22 16:04 mpvl

I will leave @mpvl to review the change itself, @kcburge, but just a comment on the DCO. The only requirement to assert the DCO is to include the trailer Signed-off-by in the commit message (per the contribution guide). So one change that will be required here is to remove the DCO text itself from the commit message.

myitcv avatar Apr 28 '22 06:04 myitcv

@mvdan strange. Not sure what's going on here, because it looks ok to me. Indeed git can also log the trailer:

git log --pretty='%h %an %(trailers:key=Signed-off-by)'

@kcburge - please can you tweak the first line of the commit message per @mpvl's change to the title of this PR?

cue: float64 support for zero

This is inline with the contribution guide on commit messages.

Perhaps a force-push on the fixed commit message will also fix the DCO check... we'll see.

myitcv avatar May 16 '22 08:05 myitcv

@kcburge friendly ping about Paul's comment above, when you have the time :) That way we can import this change into Gerrit and merge it.

If you don't have the time to do that in the next week or two, no worries - I can always take over this change for you if you'd prefer.

mvdan avatar May 31 '22 21:05 mvdan

Any update on this? Honestly, as nice as the idea/vision of CUE is, with issues like this it comes across as very flimsy and unusable in practice. 😔

curio77 avatar Feb 21 '23 12:02 curio77

Figured out the DCO error; the commit author had the email [email protected], but the DCO had [email protected]. The author seems to use the latter for his open source commits, and the signature is what matters more as far as I can tell. We haven't head from Kevin in a while, and we want this fix to be merged, so I've gone ahead and pushed to his fork branch to rebase the change and fix up the commit author and message.

mvdan avatar Apr 03 '23 16:04 mvdan