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

Add NewFromInt #72

Merged
merged 1 commit into from
Nov 29, 2019
Merged

Add NewFromInt #72

merged 1 commit into from
Nov 29, 2019

Conversation

mathieupost
Copy link
Contributor

Resolves #11

@victorquinn
Copy link
Contributor

victorquinn commented Jan 1, 2018

First, thanks for the additions!

However, is there a reason you chose newFromInt which takes an int64 and newFromInt32 which takes an int?

Why not newFromInt which takes an int and newFromInt64 which takes an int64? Seems like that would be a lot more intuitive and the method names would actually align with what they take.

If there are precedents elsewhere for this kind of thing elsewhere I could be convinced that it's a good idea, but on my first glance it seems rather confusing to have newFromInt which doesn't take an int and newFromInt32 which takes an int and no newFromInt64

Naming aside, this addition looks great and I'm happy to have more "newFroms" in Decimal!

@victorquinn
Copy link
Contributor

Just following up here, any reply to the above @mathieupost ?

@mathieupost
Copy link
Contributor Author

mathieupost commented Jan 24, 2018

Sorry for the late reply, I'm on a holiday at the moment ;)
I named it this way because that was how it was named in #11.
And I thought that since int64 is used more as far as I know, it would be cleaner in most code to be able to simply use newFromInt for those.
(also NewFromFloat uses float64)

@mathieupost
Copy link
Contributor Author

@victorquinn any comment on this?

@Mungrel
Copy link

Mungrel commented Jan 14, 2019

I agree that NewFromInt(v int), NewFromInt32(v int32) and NewFromInt64(v int64) would be a more intuitive API.

@mathieupost
Copy link
Contributor Author

Since NewFromFloat uses float64 and most libraries use int64 for integers I'll just keep the NewFromInt(int64) method and remove the NewFromInt32(int32) method, since then it'll be consistent with the rest of the API of this library.

@mathieupost mathieupost changed the title Add NewFromInt and NewFromInt32 Add NewFromInt Jan 15, 2019
@Mungrel
Copy link

Mungrel commented Jan 15, 2019

Looks like you included your .idea/ directory

@mathieupost
Copy link
Contributor Author

Oops, fixed ;)

@k1ng440
Copy link

k1ng440 commented Sep 25, 2019

please merge it

@mwoss
Copy link
Member

mwoss commented Nov 26, 2019

Hi @mathieupost, a new maintainer here. Thanks for your contribution.
May I know what will be the purpose of NewFromInt method when New method exactly matches the current use case?
If we want to create a more flexible API, maybe we should add NewFromInt32 too? What do you think?
FYI. @njason

@mathieupost
Copy link
Contributor Author

Hi! This is a long time ago 😅

I guess the idea was that with the New method you have to specify the exponent to 0 which is then kind of a magic number in the code of people using this lib. To me NewFromInt would be more clear when reading code where this lib is used.

@mwoss
Copy link
Member

mwoss commented Nov 26, 2019

I think your point is pretty valid.
What's your opinion about adding both NewFromInt32 and NewFromInt initializer methods? It will match already existing FromFloat methods.

@mathieupost
Copy link
Contributor Author

I could add NewFromInt32 again. I guess it would indeed be nice to match the Float methods.

Use NewFromInt in example in the README

Change to big.NewInt()

Add TestNewFromInt and remove NewFromInt32

Fix test

Add NewFromInt32 again
@mwoss mwoss requested review from njason and mwoss November 27, 2019 19:33
@mwoss
Copy link
Member

mwoss commented Nov 27, 2019

IMO, looks legit. : ) Let's wait for Jason's feedback

@njason njason merged commit bc70c3b into shopspring:master Nov 29, 2019
fairyhunter13 added a commit to fairyhunter13/decimal that referenced this pull request Jul 12, 2020
Add NewFromInt and NewFromInt32
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.

NewFromInt, NewFromInt32 and other convenience constructors
6 participants