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

Switch to toml-edit for spanned deserialization #2074

Merged
merged 7 commits into from
Oct 25, 2024
Merged

Switch to toml-edit for spanned deserialization #2074

merged 7 commits into from
Oct 25, 2024

Conversation

jneem
Copy link
Member

@jneem jneem commented Oct 21, 2024

This little experiment shows that we can use toml-edit for spanned parsing instead of the toml crate. It doesn't actually add any dependencies, since toml uses toml_edit for parsing anyway.

Copy link
Contributor

github-actions bot commented Oct 21, 2024

🐰 Bencher Report

Branch2074/merge
Testbedubuntu-latest

⚠️ WARNING: The following Measure does not have a Threshold. Without a Threshold, no Alerts will ever be generated!

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds CLI flag.

Click to view all benchmark results
BenchmarkLatencynanoseconds (ns)
fibonacci 10📈 view plot
⚠️ NO THRESHOLD
492,970.00
foldl arrays 50📈 view plot
⚠️ NO THRESHOLD
1,718,900.00
foldl arrays 500📈 view plot
⚠️ NO THRESHOLD
6,657,600.00
foldr strings 50📈 view plot
⚠️ NO THRESHOLD
7,194,900.00
foldr strings 500📈 view plot
⚠️ NO THRESHOLD
62,789,000.00
generate normal 250📈 view plot
⚠️ NO THRESHOLD
45,623,000.00
generate normal 50📈 view plot
⚠️ NO THRESHOLD
2,095,500.00
generate normal unchecked 1000📈 view plot
⚠️ NO THRESHOLD
3,479,200.00
generate normal unchecked 200📈 view plot
⚠️ NO THRESHOLD
767,850.00
pidigits 100📈 view plot
⚠️ NO THRESHOLD
3,230,800.00
pipe normal 20📈 view plot
⚠️ NO THRESHOLD
1,500,900.00
pipe normal 200📈 view plot
⚠️ NO THRESHOLD
10,229,000.00
product 30📈 view plot
⚠️ NO THRESHOLD
832,330.00
scalar 10📈 view plot
⚠️ NO THRESHOLD
1,514,000.00
sum 30📈 view plot
⚠️ NO THRESHOLD
830,770.00
🐰 View full continuous benchmarking report in Bencher

@jneem jneem marked this pull request as ready for review October 24, 2024 14:31
@jneem jneem changed the title Proof of concept using toml-edit Switch to toml-edit for spanned deserialization Oct 24, 2024
@jneem jneem requested a review from yannham October 24, 2024 14:31
Copy link
Member

@yannham yannham left a comment

Choose a reason for hiding this comment

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

Purely stylistic nitpick: when I start to have 3 or more (xxx_to_yyy), I now don't hesitate to declare a local, private trait (it's a bit more LoC but it feels less icky than having to call different combinations of methods based on the type). It's just a suggestion, it's also fine as it is.

Otherwise, I think a bit more doc would be nice. Not necessarily on conversion methods that are self-explanatory, but at least at the module level: if I'm a new contributor, I wouldn't necessarily understand immediately why we would do toml deserialization differently and using this lower level API while we have the serde machinery.

@@ -27,7 +27,6 @@ doc = ["dep:comrak"]
format = ["dep:topiary-core", "dep:topiary-queries", "dep:tree-sitter-nickel"]
metrics = ["dep:metrics"]
nix-experimental = [ "dep:cxx", "dep:cxx-build", "dep:pkg-config" ]
spanned-deser = ["dep:serde-untagged"]
Copy link
Member

Choose a reason for hiding this comment

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

Ah, nice, we can even get rid of the feature.

@jneem
Copy link
Member Author

jneem commented Oct 25, 2024

Purely stylistic nitpick: when I start to have 3 or more (xxx_to_yyy), I now don't hesitate to declare a local, private trait

Yes, that turned out nicely (and also removed the asymmetry between _to_term and _to_rich_term)

@jneem jneem added this pull request to the merge queue Oct 25, 2024
Merged via the queue into master with commit 0101a76 Oct 25, 2024
5 checks passed
@jneem jneem deleted the toml-edit branch October 25, 2024 16:01
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