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

Add negative numbers support #88

Merged
merged 3 commits into from
May 1, 2024
Merged

Conversation

Joraeuw
Copy link
Contributor

@Joraeuw Joraeuw commented Apr 25, 2024

Might not be the prettiest test or possibly the cleanest solution but it worked out for me. I was in a hurry but you can refactor it as you see fit :D

@martinthenth
Copy link
Owner

Hey, thanks for your PR as a fix to #87! The test looks good. I haven't dived into the issue, but I think the fix is a little too high in the code evaluation. AFAIS this fix will run on every field, whereas it would be preferred run it only for float values (and integers if they also fail).

@martinthenth martinthenth self-requested a review April 26, 2024 08:59
@Joraeuw
Copy link
Contributor Author

Joraeuw commented Apr 26, 2024

I agree that it's a little high. I wanted to catch all Integers, decimals and floats (all three used to break on negatives), instead of doing it separately for each type. I will check out your suggestion later

@martinthenth
Copy link
Owner

Thanks for the update. I reviewed your code and the solution itself looks good to me. I think the place where the code is run can be a little more precise, by evaluating only for :integer, :float, :decimal etc with something like this:

defp generate_schema({:optional, _lines, [field, type, options]})
      when type in [:integer, :float, :decimal] do
  {new_options, _} = Code.eval_quoted(options)
  %{field => [{:type, type} | new_options]}
end

Because pattern matches are faster than new function calls so this would limit the function call to those specific types. WDYT?

@Joraeuw
Copy link
Contributor Author

Joraeuw commented May 1, 2024

Yeah, I completely agree with you. I haven't really considered what is faster in elixir because I have been mostly working on high level stuff (like API's, GenServers FSMs etc.). I will change the code as per your recommendation. Might I ask, where do you get this information from? I have read a couple of books and posts but none have considered performance. There would be a line like "pattern matching is fast" but that's mostly it.

@martinthenth
Copy link
Owner

Pattern matching is O(1) time (constant) so that's going to be fastest approach in most cases. In this case, it skips execution for types not in :integer, :float, :decimal which is always beneficial

@martinthenth
Copy link
Owner

Your change looks good! 👏🏻 Thank you for your contribution to fix #87. I'll merge it and prepare a release soon.

@martinthenth martinthenth merged commit c3ca480 into martinthenth:main May 1, 2024
1 check passed
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.

3 participants