Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix failing tests from the toml-test-harness #11

Open
2 tasks
zeenix opened this issue Nov 22, 2024 · 7 comments
Open
2 tasks

Fix failing tests from the toml-test-harness #11

zeenix opened this issue Nov 22, 2024 · 7 comments

Comments

@zeenix
Copy link
Owner

zeenix commented Nov 22, 2024

#10 added tests to test the parser against the toml-test-harness suit. However, 210 tests currently fail and were ignored. The task would be to:

  • fix them all and
  • remove them from the ignore list.
@zeenix
Copy link
Owner Author

zeenix commented Nov 22, 2024

cc @CosmicHorrorDev

zeenix added a commit that referenced this issue Nov 23, 2024
This fixes a bunch of issues with number parsing and brings us a few steps
closer to #11.

Fixes #9.
@CosmicHorrorDev
Copy link
Contributor

CosmicHorrorDev commented Nov 24, 2024

I'd be willing to take a stab at getting basic quoted string parsing up to spec next

@zeenix
Copy link
Owner Author

zeenix commented Nov 24, 2024

I'd be willing to take a stab at getting basic quoted string parsing up to spec next

That's great but after working on #9, I realized that it would be probably best to take and adapt most of the parsing code from toml_edit crate. The parsing parts seems very standalone and don't drag in extra deps. That code already passes all the tests and will be a much faster way to get to resolve this issue.

@zeenix
Copy link
Owner Author

zeenix commented Nov 24, 2024

The parsing parts seems very standalone and don't drag in extra deps.

actually spoke too soon. :) Some of that code (especially the strings parser) is quite involved. So carry on please. If if you can learn from and/or take some of toml_edit code, that's not a problem still.

@CosmicHorrorDev
Copy link
Contributor

I've also got a mostly spec compliant handwritten toml-parser (10 failing tests from toml-test) that may be a good more barebones reference, but ya I've also been referencing toml_edit too 😋

@CosmicHorrorDev
Copy link
Contributor

CosmicHorrorDev commented Nov 25, 2024

Speaking of, I guess that we could lift that parser into here to further drop the number of dependencies, but there's definitely some cleanup that needs to happen first (particularly around the complete lack of error context)

@zeenix
Copy link
Owner Author

zeenix commented Nov 25, 2024

I've also got a mostly spec compliant handwritten toml-parser (10 failing tests from toml-test)

Nice. I'm glad you already know TOML so well (unlike me). :)

Speaking of, I guess that we could lift that parser into here to further drop the number of dependencies, but there's definitely some cleanup that needs to happen first (particularly around the complete lack of error context)

Hm.. while dropping our only mandatory dep does sound appealing, I think I'd rather want to continue with winnow since:

  • It's a well-established crate for parsing and chances of projects already depending on it, are very high and so in many cases, it will not even count as as an additional dependency dragged in by this crate.
  • It doesn't drag in other mandatory deps.
  • any optimisations already in it and that will come in the future, will benefit us as well. We already benefit from its simd feature.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants