Skip to content
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

1.18 Generics #103

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

1.18 Generics #103

wants to merge 5 commits into from

Conversation

Rhymond
Copy link
Owner

@Rhymond Rhymond commented Mar 31, 2022

Add 1.18 generics to go-money to allow working with different numeric types of Money (e.g. int, int64)

  • JSON marshal/unmarshal not yet working properly
  • Write tests to make sure all numeric types defined in Numeric interface are tested

@nikolaydubina
Copy link

nikolaydubina commented Apr 8, 2022

Hold up, you are not restricting type anywhere. Users may pass say float64, and loose the main benefit of this library — decimal integer arithmetics.

If adding generics, please restrict only to integer types.

Also, lots of enterprise code is not migrated to 1.18 yet. By enforcing 1.18 (breaking change?) lots of people can not use this library anymore. Please make new version or advise how this library still can be used with 1.13.

Generics are nice, but afraid would be dangerous for this project.

@Rhymond
Copy link
Owner Author

Rhymond commented Apr 8, 2022

Hold up, you are not restricting type anywhere. Users may pass say float64, and loose the main benefit of this library — decimal integer arithmetics.

If adding generics, please restrict only to integer types.

Also, lots of enterprise code is not migrated to 1.18 yet. By enforcing 1.18 (breaking change?) lots of people can not use this library anymore. Please make new version or advise how this library still can be used with 1.13.

Generics are nice, but afraid would be dangerous for this project.

Hi @nikolaydubina
There is a type constrain added :
https://github.com/Rhymond/go-money/pull/103/files#diff-d48dd271ebf1115ce9c4ee56a566b1d006eee1bc2e3211630b302c717464f964R70

type Numeric interface {
	~int | ~int8 | ~int16 | ~int32 | ~int64
}

It only allows different types of int, still working on implementing uint and possibly bigint (going to create a new PR for that)

At the start I was thinking to roll up a new major version, but it looks like that existing tests are all passing fine, which means there are no breaking changes. There are still work needed to be done in regards to custom json marshalling.

@totemcaf
Copy link
Contributor

@nikolaydubina
What's the benefit of making money generic this way? The only one I can see is to save a little of space. The performance will not be better in modern processors, and memory is relatively cheap. It also introduce complexity in its use.

Additionally, is a money based on "int8" useful?

@nikolaydubina
Copy link

The benefit is that it is not possible to pass float types. One of selling points for go-money is numerical precision, which is gone once floats are used internally.

the performance will not be better in modern processors, and memory is relatively cheap. It also introduce complexity in its use.

yes

is a money based on "int8" useful?

not really, unless you running on some special hardware. but then likely you would be using C instead

But then again, please check with project owner to align on this! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants