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

move from lexical-core to depend on lexical-parse-float directly #57

Merged
merged 4 commits into from
Jan 22, 2024
Merged

move from lexical-core to depend on lexical-parse-float directly #57

merged 4 commits into from
Jan 22, 2024

Conversation

decathorpe
Copy link
Contributor

@decathorpe decathorpe commented Jan 18, 2024

Closes #49.

Code changes are minimal - I have adjusted the use statement to import things under the same names as previously, and only one line of actual code needs to be changed - the parse_partial_with_options function is from lexical-core, but it is a trivial one-line function, so it can just be replaced with calling the trait method on f64 directly.

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

Merging #57 (789123c) into main (29b5833) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #57   +/-   ##
=======================================
  Coverage   95.35%   95.35%           
=======================================
  Files           8        8           
  Lines        1097     1097           
=======================================
  Hits         1046     1046           
  Misses         51       51           
Files Coverage Δ
src/number_decoder.rs 90.52% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29b5833...789123c. Read the comment docs.

Copy link

codspeed-hq bot commented Jan 20, 2024

CodSpeed Performance Report

Merging #57 will not alter performance

Comparing decathorpe:main (789123c) with main (29b5833)

Summary

✅ 54 untouched benchmarks

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'll let @davidhewitt review and approve.

@decathorpe
Copy link
Contributor Author

Sorry about the formatting, I forgot to run rustfmt after my changes.

If I read the benchmark results correctly, this small change actually speeds things up measurably? That's interesting ... well, it's one fewer function call, so the code can probably be more optimized.

@samuelcolvin
Copy link
Member

samuelcolvin commented Jan 20, 2024

I'm not sure if we have good float benchmarks? we do have some.

@davidhewitt
Copy link
Collaborator

LGTM, thanks!

@davidhewitt davidhewitt merged commit 1c6e5b5 into pydantic:main Jan 22, 2024
10 checks passed
@davidhewitt
Copy link
Collaborator

To summarise my opinion here: I think at some point we might want to remove the lexical dependency entirely, but this is good enough for downstream packaging for now. So we can postpone the bigger work until we have the resources to also maintain performance.

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.

Consider migrating away from the lexical_core crate
3 participants