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

use BigInt instead of Int #40

Closed
wants to merge 4 commits into from
Closed

Conversation

gbagan
Copy link
Collaborator

@gbagan gbagan commented May 29, 2022

This ia PR following the advice of @hdgarrood #33

The changes are:

  • Rational is now a newtype over Ratio BigInt.
  • Add a dependency on bigints and remove the dependency on integers
  • numerator and denominator now return BigInts
  • % is polymorphic. It accepts Int -> Int -> Rational or BigInt -> BigInt > Rational. This minimizes breaks.
  • Update the tests

The readme needs to be updated.

Copy link

@f-f f-f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested this and it works, so I think it's good to merge

@f-f
Copy link

f-f commented Mar 22, 2023

The only things to do I think should be:

  • resolving conflicts
  • noting somewhere (maybe add a package.json?) that this requires installing the big-integers JS package

@sigma-andex
Copy link

I think it would be better to make this depend on js-bigints, which doesn't require a npm dependency.

@gbagan
Copy link
Collaborator Author

gbagan commented Mar 22, 2023

I think the same thing now that bigints are widely adopted by browsers.
js-bigints does not require a npm dependency, is faster and easier to maintain.
Maybe we should wait for js-bigints to be transferred to purescript-contrib.

@sigma-andex
Copy link

Maybe we should wait for js-bigints to be transferred to purescript-contrib.

Done! https://github.com/purescript-contrib/purescript-js-bigints

@f-f
Copy link

f-f commented Apr 19, 2023

Any news on this?
@gbagan I could help with moving forward this branch if needed

@gbagan
Copy link
Collaborator Author

gbagan commented Apr 19, 2023

Hi,

Sorry for the delay.
Actually, I just need a new realease of js-bigints (which contains the last PR with toInt and toNumber).

@f-f
Copy link

f-f commented Apr 24, 2023

New release is out! (thanks @sigma-andex)

@gbagan
Copy link
Collaborator Author

gbagan commented May 3, 2023

Since js-bigints 2.1 is now present in the last package-set, I have replaced bigints with js-bigints as object behind Rational

@gbagan
Copy link
Collaborator Author

gbagan commented May 3, 2023

@f-f or someone else,
could you help me to resolve the conflicts, please?
I don't know what to do

@gbagan
Copy link
Collaborator Author

gbagan commented May 3, 2023

I have created a branch gbagan-master that resolve conflicts.
Do I need to make a new PR with this branch?

@f-f
Copy link

f-f commented May 3, 2023

You could merge the new branch in here, but making a new PR is just easier! I can review that right away

@gbagan gbagan mentioned this pull request May 3, 2023
@f-f
Copy link

f-f commented May 3, 2023

Closing in favour of #42

@f-f f-f closed this May 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants