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

Start migration from float64 to big.Float #232

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

Conversation

angshumanHalder
Copy link
Contributor

@angshumanHalder angshumanHalder commented Jan 24, 2023

@angshumanHalder angshumanHalder marked this pull request as draft January 24, 2023 06:55
@github-actions
Copy link

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Jan 24, 2023
@gavv
Copy link
Owner

gavv commented Jan 24, 2023

Hi, please rebase your PR on fresh master. It has reworked chain interface.

Instead of:

	r.chain.enter("XXX()")
	defer r.leave()

	if r.failed() {
		return r
	}

we should now use:

	opChain := r.chain.enter("Decode()")
	defer opChain.leave()

	if opChain.failed() {
		return r
	}

In other words, chain.enter() now returns a new chain instance, and we should use it for all other operations with chain. This change is part of making this lib thread-safe.

Related commits:

@gavv gavv added the work in progress Pull request is still in progress and changing label Jan 24, 2023
@gavv
Copy link
Owner

gavv commented Jan 24, 2023

BTW if you don't mind, could you rollback reordering of methods in this PR? No problem with reordering itself, but it makes the diff a mess and it's hard to tell what changed exactly.

@angshumanHalder
Copy link
Contributor Author

Sure. Would rollback the ordering. Actually When I raised this PR initially I had conflicts which upon resolving messed up the ordering of the functions. I will revert it to initial ordering.

@gavv gavv force-pushed the master branch 2 times, most recently from 43b0a86 to ee27d9f Compare January 27, 2023 08:20
@gavv gavv linked an issue Jan 30, 2023 that may be closed by this pull request
@gavv gavv removed the needs rebase Pull request has conflicts and should be rebased label Feb 3, 2023
@gavv gavv force-pushed the master branch 5 times, most recently from d159043 to 7d9207b Compare February 4, 2023 11:47
@github-actions
Copy link

github-actions bot commented Feb 8, 2023

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Feb 8, 2023
@gavv gavv added ready for review Pull request can be reviewed and removed needs rebase Pull request has conflicts and should be rebased labels Apr 8, 2023
@github-actions
Copy link

github-actions bot commented Apr 9, 2023

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Apr 9, 2023
@gavv gavv removed the needs rebase Pull request has conflicts and should be rebased label Apr 9, 2023
@gavv
Copy link
Owner

gavv commented Apr 10, 2023

Could you please revert unrelated changes (like formatting) in tests? This is a big pr, and it makes reviewing harder.

@gavv
Copy link
Owner

gavv commented Apr 10, 2023

Also please remove commented code

@gavv
Copy link
Owner

gavv commented Apr 10, 2023

Could you also provide a short summary of what is addressed by this pr and what is not?

@gavv gavv changed the title WIP: Migration from float64 to big.Float Stary migration from float64 to big.Float Apr 10, 2023
@gavv gavv changed the title Stary migration from float64 to big.Float Start migration from float64 to big.Float Apr 10, 2023
@github-actions
Copy link

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Apr 10, 2023
@angshumanHalder
Copy link
Contributor Author

Sure. Let me resolve conflicts and revert formatting changes. Give me few days.

@gavv gavv added work in progress Pull request is still in progress and changing and removed ready for review Pull request can be reviewed labels Apr 11, 2023
@angshumanHalder
Copy link
Contributor Author

angshumanHalder commented Apr 13, 2023

Hey @gavv reverted the formatting changes.

Also short summary -

  1. json.Unmarshal() is removed and instead we are using json.Decode()
  2. Numbers are converted to big.Float from json.Number() only when there is a precision loss otherwise default float64 is used.
  3. numbers are represented in big.Float internally.

@gavv gavv added ready for review Pull request can be reviewed and removed work in progress Pull request is still in progress and changing needs rebase Pull request has conflicts and should be rebased labels Apr 22, 2023
@github-actions
Copy link

github-actions bot commented Oct 1, 2023

☔ The latest upstream change (presumably these) made this pull request unmergeable. Please resolve the merge conflicts.

@github-actions github-actions bot added the needs rebase Pull request has conflicts and should be rebased label Oct 1, 2023
@angshumanHalder angshumanHalder force-pushed the 191-migrate-from-float64-to-big.Float branch from af44eb7 to ca0e087 Compare January 20, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Pull request has conflicts and should be rebased ready for review Pull request can be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Number from float64 to big.Float
3 participants