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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace Number enum with Option<f32> #83

Closed
alice-i-cecile opened this issue Jun 8, 2022 · 3 comments 路 Fixed by #144
Closed

Replace Number enum with Option<f32> #83

alice-i-cecile opened this issue Jun 8, 2022 · 3 comments 路 Fixed by #144
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier.

Comments

@alice-i-cecile
Copy link
Collaborator

alice-i-cecile commented Jun 8, 2022 •
edited

The entire Number enum appears to be an unidiomatic reimplementation of Option<f32> (or perhaps a Result).

Remove this type, and replace it throughout the code base.

This is breaking, because this type is public 馃ゲ.
@alice-i-cecile alice-i-cecile added good first issue Good for newcomers code quality Make the code cleaner or prettier. breaking-change A change that breaks our public interface labels Jun 8, 2022
@alice-i-cecile alice-i-cecile changed the title Replace Number enum with Option<f32> Replace Number enum with Result<f32> Jun 8, 2022
@alice-i-cecile
Copy link
Collaborator Author

Overall I think this is more appropriately handled by a Result type.

We could also consider using an existing ecosystem crate that wraps an f32 and forbids NaN values.

@alice-i-cecile alice-i-cecile removed the good first issue Good for newcomers label Jun 8, 2022
@alice-i-cecile alice-i-cecile mentioned this issue Jun 8, 2022
Closed
@TimJentzsch TimJentzsch mentioned this issue Jun 9, 2022
Merged
4 tasks
@TimJentzsch
Copy link
Collaborator

Why do you think this is better handled by a Result? As far as I understand it just indicates that the size is not set for the given node, so IMO the error doesn't make sense here.

@alice-i-cecile
Copy link
Collaborator Author

Yeah, I had expected this to have protections against NaN, but it doesn't. Option is preferred

@alice-i-cecile alice-i-cecile changed the title Replace Number enum with Result<f32> Replace Number enum with Option<f32> Jun 11, 2022
@alice-i-cecile alice-i-cecile mentioned this issue Jun 11, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
breaking-change A change that breaks our public interface code quality Make the code cleaner or prettier.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Replace Number type with a simpler Option<f32> alice-i-cecile/taffy
2 participants
@alice-i-cecile @TimJentzsch