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.

Sign up for GitHub

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

Jump to bottom

fix: Handle bigints properly in more cases #3345

Merged
merged 5 commits into from
Jan 24, 2025
Merged

fix: Handle bigints properly in more cases #3345

merged 5 commits into from
Jan 24, 2025

Conversation

gwhitney
Copy link
Collaborator

@gwhitney gwhitney commented Jan 3, 2025

  • Allow a bigint to appear in a max/min of nonhomogeneous arguments
  • If randomInt is called with a bigint or a pair of bigints, return a bigint.
  • Preserve uniformity of results if randomInt is called with a very large range.
  • Extend log, log2, and log10 to bigints.
  • Allow mixed Fraction and BigNumber in smaller and larger.
  • Add tests for all of the above issues.
  • Bonus: fix one JSDoc comment issue. If every PR fixes one, we will soon get through the 150+ of them.

Resolves #3344.

@gwhitney
fix: Handle bigints properly in more cases
749f07a 
  * Allow a bigint to appear in a max/min of nonhomogeneous arguments
  * If randomInt is called with a bigint or a pair of bigints, return
    a bigint.
  * Preserve uniformity of results if randomInt is called with a very large
    range.
  * Extend log, log2, and log10 to bigints
  * Add tests for all of the above issues.
  * Bonus: fix one JSDoc comment issue. If every PR fixes one, we will soon
    get through the 150+
@gwhitney gwhitney mentioned this pull request Jan 3, 2025
Draft
@josdejong josdejong mentioned this pull request Jan 8, 2025
Closed
josdejong
josdejong reviewed Jan 9, 2025
View reviewed changes
Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Thanks @gwhitney, looks good!

I have made a few comments with some questions and minor feedback, can you have a look at those?

src/function/arithmetic/log.js Outdated Show resolved Hide resolved
src/function/probability/randomInt.js Show resolved Hide resolved
src/function/relational/larger.js Outdated Show resolved Hide resolved
src/function/relational/smaller.js Outdated Show resolved Hide resolved
@gwhitney
Copy link
Collaborator Author

OK, I believe I have incorporated all feedback and gotten this PR back into a good state. Thanks for your review.

josdejong
josdejong approved these changes Jan 24, 2025
View reviewed changes
Copy link
Owner

@josdejong josdejong left a comment

Choose a reason for hiding this comment

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

Thanks for the updates Glen, I'll merge the PR now

@josdejong josdejong merged commit 8510b85 into josdejong:develop Jan 24, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josdejong josdejong josdejong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

instances of lack of support for bigints
2 participants
@gwhitney @josdejong