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

Do not use BigDecimal.new to construct #543

Closed
wants to merge 1 commit into from
Closed

Do not use BigDecimal.new to construct #543

wants to merge 1 commit into from

Conversation

headius
Copy link
Contributor

@headius headius commented Oct 9, 2023

@headius headius mentioned this pull request Oct 9, 2023
Closed
@hsbt
Copy link
Collaborator

hsbt commented Oct 11, 2023

@headius I fixed CI failure with #544. Can you rebase this from HEAD?

@headius
Copy link
Contributor Author

headius commented Oct 13, 2023

@hsbt Done!

@kares
Copy link
Contributor

kares commented Oct 15, 2023 •
edited

while this is 👍, it's also breaking in terms of the decimal_class feature, which is an option JSON still supports.

@kares kares mentioned this pull request Oct 15, 2023
Merged
@kares
Copy link
Contributor

kares commented Oct 15, 2023

building on the changes here, I've come up with: #546 ... which should be "more" 🔙wards compatible

@hsbt
Copy link
Collaborator

hsbt commented Oct 17, 2023

@headius @kares I have no idea to merge this or #546.

@kares
Copy link
Contributor

kares commented Oct 17, 2023 •
edited

I would obviously vote for #546 but let's wait for @headius to do a review.

The main difference between the two is that this PR will not consider a user provided custom decimal_class option, while #546 will.

@headius
Copy link
Contributor Author

headius commented Oct 17, 2023

#546 should be merged as it includes these fixes while presenting backward compatibility. Thanks @kares !

@headius headius closed this Oct 17, 2023
@headius headius deleted the no_bigdecimal_new branch October 17, 2023 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update JSON gem to not use Bigdecimal.new
3 participants
@headius @hsbt @kares