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

Implement float parsing fixes #6 #11

Closed
wants to merge 1 commit into from

Conversation

terrarier2111
Copy link
Contributor

@terrarier2111 terrarier2111 commented Jan 14, 2024

Note that things like 1.2 when made to_string spit out incorrect results (like 1.199999999) but this seems to be an issue in the to_string impl as the same happens with from_f64(1.2)

@terrarier2111 terrarier2111 changed the title Implement float parsing Implement float parsing fixes #6 Jan 14, 2024
@terrarier2111
Copy link
Contributor Author

The test failure seems to be spurious and unrelated to this feature

@nadavrot
Copy link
Owner

@terrarier2111 Thanks for working on it. I love it. Let's move forward in a few steps.
Can you first submit the cleanup commits as a separate pull request? The stuff for adding const and the other cleanups should be easy to review and land.

I read the code and I have a few comments. First, we'll need a little bit more comments and a few tests. And second, I'd like to talk about the API for constructing a float number. I noticed that you added a default semantics, but I think that we should have something like Float::from_string. What do you think?

@terrarier2111
Copy link
Contributor Author

terrarier2111 commented Jan 15, 2024

@terrarier2111 Thanks for working on it. I love it. Let's move forward in a few steps. Can you first submit the cleanup commits as a separate pull request? The stuff for adding const and the other cleanups should be easy to review and land.

I read the code and I have a few comments. First, we'll need a little bit more comments and a few tests. And second, I'd like to talk about the API for constructing a float number. I noticed that you added a default semantics, but I think that we should have something like Float::from_string. What do you think?

Hey, thank you for your reponse, i cleaned up the impl a bit, fixed some issues and added tests, tomorrow i will move all the code in a function that takes a semantic as an argument and let the try_from impl call this function with some default, at least for now until i implement autodetecting semantics. Are you okay with this moving forward or do you have some changes that you want? Regarding the additional changes that are unrelated to this pr i just found them randomly, i wouldn't mind removing them but i can't promise that i'll do them again as i am not really good with git and it is probably a bit cumbersome. Also i named the method try_from_str idk if this is better or just from_str/from_string

@nadavrot
Copy link
Owner

Can you please land the cleanup commits as a separate PR, one by one? It would be easier to review and land.

Afterwards I'll be able to review this diff. I think that you are really close, and I only have small suggestions (such as moving the tests into a new function, no big deal). You are almost there, just split the PR into smaller diffs.

@terrarier2111
Copy link
Contributor Author

Can you please land the cleanup commits as a separate PR, one by one? It would be easier to review and land.

Afterwards I'll be able to review this diff. I think that you are really close, and I only have small suggestions (such as moving the tests into a new function, no big deal). You are almost there, just split the PR into smaller diffs.

I removed the cleanup changed and moved the tests

@terrarier2111
Copy link
Contributor Author

terrarier2111 commented Jan 25, 2024

@nadavrot are you still reviewing this or can i close this and just use my fork?

@nadavrot
Copy link
Owner

I would love for you to land the chances, but not in this pull request. Please close this PR and split the changes here into separate pull requests with cleanups and afterwards with the changes for landing the float parse. Each PR should have one commit. Thank you. I really appreciate your contribution and would love to see it land, one PR at a time.

@terrarier2111
Copy link
Contributor Author

I would love for you to land the chances, but not in this pull request. Please close this PR and split the changes here into separate pull requests with cleanups and afterwards with the changes for landing the float parse. Each PR should have one commit. Thank you. I really appreciate your contribution and would love to see it land, one PR at a time.

I think i did it, it's now only a single commit!

@nadavrot
Copy link
Owner

nadavrot commented Jan 27, 2024

Please submit a new and clean pull request (without the previous discussion).

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.

2 participants