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

Remove boilerplates and fix bugs #23

Merged
merged 1 commit into from
Nov 17, 2022

Conversation

ryo33
Copy link
Contributor

@ryo33 ryo33 commented Nov 17, 2022

Requires: jsinger67/parol_runtime#2

I removed the boilerplates by using re-exports from parol_runtime.
Also, I fixed two bugs:

Copy link
Owner

@jsinger67 jsinger67 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@jsinger67 jsinger67 merged commit ef44da9 into jsinger67:main Nov 17, 2022
@jsinger67
Copy link
Owner

Thanks a lot for your great work 🥇

@jsinger67
Copy link
Owner

I have problems with generated crates via parol new subcommands that relate to the removal of dependencies in user crates.
error[E0433]: failed to resolve: could not find derive_builder in the list of imported crates

  • parol_runtime is updated to version 0.9.0
  • parol's changes are on main branch

Please have a look at this.

To reproduce this, please issue the command
parol new -b -p .\my-grammar
then
cd .\my-grammar\
and
cargo build

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 18, 2022

Thanks for merging!

Please have a look at this.

I'll try to fix that as soon as possible.

@ryo33 ryo33 deleted the remove-boilerplates branch November 18, 2022 04:51
@ryo33
Copy link
Contributor Author

ryo33 commented Nov 18, 2022

Fixed in #24

@jsinger67
Copy link
Owner

@ryo33, I experienced a problem with features of crates that are re-exported for reuse from parol_runtime crate.
I used to enable the feature fancy of miette to get useful error reports in crates generated by parol new like this:

Error: parol_runtime::parser::unprocessed_input

× Failed parsing file ./test.txt
╰─▶ Unprocessed input is left after parsing has finished
╭─[./test.txt:5:1]
5 │
6 │ # End
· ───┬──
· ╰── Last processed token
╰────
help: Unprocessed input is left after parsing has finished

But currently I get this:

Error: Diagnostic { message: "Failed parsing file ./test.txt", code: "parol_runtime::parser::unprocessed_input", help: "Unprocessed input is left after parsing has finished", labels: "[LabeledSpan { label: Some("Last processed token"), span: SourceSpan { offset: SourceOffset(86), length: SourceOffset(5) } }]" }
NOTE: If you're looking for the fancy error reports, install miette with the fancy feature, or write your own and hook it up with miette::set_hook().

The feature fancy is not enabled in parol_runtime itself, because it is not needed there, which explains the changed behavior.

I think that it isn't possible to use a re-exported crate and thereby changing the used feature set.

Do you have any idea how to deal with that?

I could simply add the line

miette = { version = "5.2.0", features = ["fancy"] }

to the crate's Cargo.toml but this seems awkward.

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 29, 2022

I have an idea of one way that might work. I will try it tomorrow.

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 29, 2022

I tried it and found it difficult at present.

rust-lang/cargo#674

@ryo33
Copy link
Contributor Author

ryo33 commented Nov 29, 2022

This is a separate discussion, but it seems a bit awkward that the generated code returns miette::Report. Since the generated parser is more of a library than an application, it looks more natural to return a dedicated error enum, as said in https://docs.rs/miette/latest/miette/#-in-libraries:

Then, return this error type from all your fallible public APIs. It’s a best practice to wrap any “external” error types in your error enum instead of using something like Report in a library.

Sometime soon, I was going to open an issue titled "Structural Errors". IMO It's sufficient to return the information necessary to produce a fancy error message as most parser combinator libraries do.

@jsinger67
Copy link
Owner

You are right.
miette gave me the opportunity to get quickly to a result. For instance, I used support from miette like miette::protocol::SourceSpan to covey the error position to the caller.

This can surely be redesigned to emphasize the role of parol_runtime and also the generated parsers as libraries.
I think it should be possible to use e.g., thiserror or anyhow as a substitution and miette, if desired, as error wrapper in applications using those parsers.
This choice should be given to the user.

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.

None yet

2 participants