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

Context for built-in combinators #421

Open
2 tasks done
judemille opened this issue Jan 12, 2024 · 2 comments
Open
2 tasks done

Context for built-in combinators #421

judemille opened this issue Jan 12, 2024 · 2 comments
Labels
A-combinator Area: combinators C-question Uncertainty is involved

Comments

@judemille
Copy link

judemille commented Jan 12, 2024

Please complete the following tasks

winnow version

0.5.34

Describe your use case

Using Winnow without adding .context(...) calls to every combinator.

Describe the solution you'd like

Context should be added to built-in combinators, so that when they produce an error, the Display impl indicates what went wrong and why, without having to trace the parsing.

I can help with implementation, once details are worked out.

Alternatives, if applicable

Forcing users to add context calls to each use of every combinator, as current, but that is really not ideal.

Additional Context

This may be difficult, due to the complexities of AddContext. I'm not sure how best to implement this.

@judemille judemille added the C-enhancement Category: Raise on the bar on expectations label Jan 12, 2024
@epage
Copy link
Collaborator

epage commented Jan 12, 2024

This was an intentional choice. toml_edit originally used combine which would automatically determine what was expected. The problem was that it was slow (because you couldn't opt-out of what we're getting in #415) and the quality was low. I found that hand-specifying the values gives much better results.

This comes at the cost of not getting great errors in more trivial / prototype cases.

The other problem with baking them in is that it would tie us to one specific type context type. Currently, we offer a default choice but allow people to customize it (which the author of #415 seems to do).

@epage epage added the A-combinator Area: combinators label Jan 12, 2024
@judemille
Copy link
Author

Yeah, I'm not sure how to solve that issue. I figure there must be a good way to go about it, but I'm at a loss.

@epage epage added C-question Uncertainty is involved and removed C-enhancement Category: Raise on the bar on expectations labels Feb 12, 2024
@9999years 9999years mentioned this issue Oct 14, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-combinator Area: combinators C-question Uncertainty is involved
Projects
None yet
Development

No branches or pull requests

2 participants