Skip to content

feat: arbitrary precision#9

Open
sodiboo wants to merge 6 commits intojoshprk:mainfrom
sodiboo:arbitrary-precision
Open

feat: arbitrary precision#9
sodiboo wants to merge 6 commits intojoshprk:mainfrom
sodiboo:arbitrary-precision

Conversation

@sodiboo
Copy link
Contributor

@sodiboo sodiboo commented Jul 27, 2025

i've made the test code a bit nicer:

  • mainly, the test output directory is now in target/tmp of the actual cargo workspace, instead of target/test_outputs in the repo root. (i have my clone of this repo in a workspace; so this was bugging me)
  • some test failures are also easier to read now. this uses the fancy feature of miette, but it's only in test code. jsonkdl itself is still minimal, depending only on serde_json and kdl.

  • i added tests for arbitrary precision, and then implemented arbitrary precision.
  • in doing so, i ended up undoing 2107289, since those two methods aren't actually separable due to value_repr being part of the KdlEntry.

sodiboo added 4 commits July 27, 2025 09:07
previously, the stderr would be interlaced with the test runner's
output, which sucks when that's the part that actually matters.

now, we capture stdout and stderr, and print it with `println`, which is
nicely displayed by the test runner
mainly because this merges their dead code evaluation, so `common` no
longer has "unused functions".

also they are literally the same test cases. why were they separated lol
@sodiboo
Copy link
Contributor Author

sodiboo commented Jul 27, 2025

lmao https://github.com/joshprk/jsonkdl/actions/runs/16548564711/job/46799978335?pr=9

 error[E0507]: cannot move out of a shared reference
  --> tests/arbitrary_precision.rs:77:47
   |
77 |         literal.parse::<i128>().map_err(|err| *err.kind()),
   |                                               ^^^^^^^^^^^ move occurs because value has type `IntErrorKind`, which does not implement the `Copy` trait
   |
help: consider cloning the value if the performance cost is acceptable
   |
77 -         literal.parse::<i128>().map_err(|err| *err.kind()),
77 +         literal.parse::<i128>().map_err(|err| err.kind().clone()),

test failure because that was just merged seemingly right before the current stable Rust. i will fix it, because this also applies to the compiler version that is currently in nixpkgs.

sodiboo added 2 commits July 27, 2025 09:41
this also undoes 2107289 because
KdlValue cannot have a custom repr, oopsie, those functions are
inseparable after all.
@sodiboo sodiboo force-pushed the arbitrary-precision branch from 627512c to 87fcfba Compare July 27, 2025 07:41
@sodiboo
Copy link
Contributor Author

sodiboo commented Jul 27, 2025

the actual tests are expected to fail when ran. as previously stated, this is due to a kdl-rs bug. but the CI did catch the fact that the tests did not compile on 1.88.0

@joshprk
Copy link
Owner

joshprk commented Aug 8, 2025

Hi, sorry for the wait. I'll take a look at this soon. I notice that the CI build didn't pass and you mentioned it, should I wait a bit longer or is it ready for review?

@sodiboo
Copy link
Contributor Author

sodiboo commented Aug 9, 2025

Yeah. This is ready for review. As stated, tests are failing due to a bug in kdl-rs which can't parse documents with arbitrary precision numbers (only output them), and my tests include round-trip tests to ensure they parse correctly. I looked manually at the test outputs; it seems correct.

No worries about the wait; i only really care about using this for nixpkgs, which doesn't support arbitrary precision anyways, so it's not like this is blocking anything. Take your time.

@joshprk
Copy link
Owner

joshprk commented Aug 10, 2025

After reviewing this, I think the best thing to do is annotate the clearly working test cases so we can compare the output text to something we verified in order to pass CI tests (since it would be unbecoming for a parsing tool to have failing CI).

It seems adding arbitrary precision to the upstream kdl-rs is non-trivial (or they can make a new KdlValue variant?), and we should instead make a tracking issue for it until they are able to address it.

Let me know if you have any concerns about annotating the arbitrary precision test cases with verified outputs that we can diff with the jsonkdl output. Otherwise, I'll co-author this PR to add the annotations. Thanks for sending in this PR!

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