cucumber / cucumber-expressions

Human friendly alternative to Regular Expressions
MIT License
148 stars 51 forks source link

Golang big.Float is not a big decimal #203

Open aaronc opened 1 year ago

aaronc commented 1 year ago

πŸ€” 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.

mpkorstanje commented 1 year ago

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.

mpkorstanje commented 1 year ago

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.

mpkorstanje commented 1 year ago

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.

aaronc commented 1 year ago

In the meantime should I submit a PR to disable the golang registration of bigdecimal?

luke-hill commented 6 months ago

Ping @mpkorstanje - I support the ruby removal

mpkorstanje commented 6 months ago

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.

mpkorstanje commented 6 months ago

@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?

kieran-ryan commented 6 months ago

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)

luke-hill commented 6 months ago

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.

mpkorstanje commented 6 months ago

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!

luke-hill commented 6 months ago

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

mpkorstanje commented 6 months ago

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.