cucumber-expressions
cucumber-expressions copied to clipboard
cucumber
Golang big.Float is not a big decimal
🤔 What's the problem you've observed?
I am considering using the golang implementation of cucumber-expressions in https://github.com/regen-network/gocuke and noticed that bigdecimal is mapped to https://pkg.go.dev/math/big#Float.
The documentation for big.Float states that this is structured as sign × mantissa × 2**exponent. This is not a decimal number! It is a base-2 floating point number, not base-10 as required for decimals.
Java does provide a proper BigDecimal where the documentation clearly states that the structure is unscaledValue × 10-scale.
✨ Do you have a proposal for making it better?
The mapping of bigdecimal to big.Float should be removed in the golang package and consumers should be allowed to register a correct decimal implementation (none is provided by the standard library).
Also in the golang implementation, it seems like it's an error to override a built-in type mapping. It might be worth reconsidering this in case users want to swap out other type mappings.
📚 Any additional context?
Two correct golang decimal implementations are: https://pkg.go.dev/github.com/cockroachdb/apd/v3 and https://pkg.go.dev/github.com/ericlagergren/decimal/v3.
This text was originally generated from a template, then edited by hand. You can modify the template here.
The mapping of bigdecimal to big.Float should be removed in the golang package and consumers should be allowed to register a correct decimal implementation (none is provided by the standard library).
That is perfectly acceptable. I'm kinda surprised that the big-decimal test case passed. I suppose we haven't quite added enough digits to it.
~~Would you be able to send pull request for this? Feel free to update the big decimal test case with enough digits to make it fail and then exclude the big decimal test case for Go. If other languages start failing we can look at those separately.~~
edit: I'm going to find a bigger consensus on this. I'd like to revert most if not all of #42.
Also in the golang implementation, it seems like it's an error to override a built-in type mapping. It might be worth reconsidering this in case users want to swap out other type mappings.
That sounds reasonable but I'd like to find a larger consensus.
Note: I find the rationale from https://github.com/cucumber/cucumber-expressions/pull/42 to add {bigdecimal} to every language rather shaky. There don't appear to have been any other alternatives considered.
The test case for big decimal also appears to be intentionally hamstrung to avoid the problems caused by a lack of precision. Compared to the big integer test case we're missing quite a few digits.
In the meantime should I submit a PR to disable the golang registration of bigdecimal?
Alright, that is enough consensus for me (esp with https://github.com/cucumber/cucumber-expressions/pull/273 in mind). Let us remove the big decimal from Go and Ruby. It should be marked as a breaking change though so we don't blindside anyone.
@xeger @kieran-ryan for Javascript we currently have the same problem. Though going by #42 this code is also used in vscode somewhere, somehow, and removing it from Javascript would break that. Do you see a better, alternative solution on the vscode side?
Though going by #42 this code is also used in vscode somewhere, somehow, and removing it from Javascript would break that. Do you see a better, alternative solution on the vscode side?
Not sure I fully understand scope of changes just yet, however, bigdecimal is referenced in each language implementation of the language service - used by the language server and vscode extension. Perhaps there may not be a pressing need to remove from the language service in the near term - given users with versions up to now would be supported along with the subset of users if this change goes through? Though based on agreement above, we would need to remove at some stage, which I would be happy to support.
However, there may be changes required to support the following - though believe should be fine if registered as a Parameter Type in glue files or extension settings, would need to double check:
consumers should be allowed to register a correct decimal implementation (none is provided by the standard library)
So basically we want to remove the BigDecimal pre-registered parameter type from several flavours of cucumber.
Any / all additional work for language server e.t.c. to not break it is also needed.
Reason -> Probably shouldn't have been added in first place, it's causing a few headaches now.
from several flavours of cucumber.
To be exact, all flavours that don't support a big decimal type.
Any / all additional work for language server e.t.c. to not break it is also needed.
Yes, but I'd like to consider those changes independently.
Though based on agreement above, we would need to remove at some stage, which I would be happy to support.
@kieran-ryan much appreciated!
So ruby does support a bigdecimal type, but it involves in importing some rather large compiled libraries, for something that likely never gets used.
The way I look at it, if you want a bigdecimal type for your scenarios, import the library and define your own
Mmh. I would consider those two scenarios to be equivalent for this discussion.
Unless you or someone else wants to spend sometime making ruby's implementation able to detect the presence of the big decimal library. And automatically add the type when the library is present.