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

Add to support load multiple grammars for derive generator. #758

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

huacnlee
Copy link
Member

@huacnlee huacnlee commented Jan 4, 2023

Resolve #197 #333

What New

Allows load multiple grammar files for derive generator.

Example:

#[derive(Parser)]
#[grammar = "base.pest"]
#[grammar = "json.pest"]
pub struct JSONParser;

By this changes, we can split the sharing rules in to a single file, and then load it for other grammar files.

In my AutoCorrect case, this will useful, because there have so many duplicate rules (e.g. space, comment, newline) defined in each grammars.

https://github.com/huacnlee/autocorrect/tree/v2.5.5/autocorrect/grammar

@huacnlee huacnlee requested a review from a team as a code owner January 4, 2023 11:33
@huacnlee huacnlee requested review from tomtau and removed request for a team January 4, 2023 11:33
@huacnlee huacnlee changed the title Add to support multiple grammars for derive generator. Add to support use load partial for Pest grammars. Jan 4, 2023
@huacnlee huacnlee force-pushed the feat/use-pest branch 2 times, most recently from fbf1f57 to 30c15cf Compare January 4, 2023 14:37
@tomtau tomtau requested review from CAD97, NoahTheDuke, a team, dragostis and glyn January 4, 2023 14:37
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

Great work, thanks! I think that multi-derive generator approach (i.e. this commit 157f159 ) is simple and can address the basic use cases, so I'd be happy to merge that right away -- perhaps could you split it into two PRs?

For the meta grammar extension (i.e. this commit 30c15cf ), I think it's fine, but would want more feedback from other folks. If my understanding of this use <filename> feature is correct, it'll simply concat all definitions into one grammar (i.e. it's unlike that previous work by @bobbbay #660 that tried to add module-level scoping). Is that correct? If so, what is the advantage of writing use ... inside a grammar over writing multiple derive annotations?

If this meta grammar extension is to be added, it's maybe worth adding more complex tests (e.g. use statements across multiple files A.pest -> B.pest -> C.pest, or multiple use statements in one file) and documenting this feature in the grammar reference in rustdocs/readme/book (especially its limitations or what one needs to watch for... I assume for this concat approach, there can't be a cycle / mutual dependency, one needs to make sure each grammar file is included only once...?).

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
@huacnlee
Copy link
Member Author

huacnlee commented Jan 4, 2023

I have splitted #759

@huacnlee huacnlee changed the title Add to support use load partial for Pest grammars. Add to support load multiple grammars for derive generator. Jan 4, 2023
@CAD97
Copy link
Contributor

CAD97 commented Jan 4, 2023

I agree that using concat for multiple #[grammar] attributes makes sense.

I'm pretty firmly against implementing a use via textual inclusion (Rust include!, C++ #include), though. At a minimum, use/import needs some “#pragma once” behavior, so you can have a diamond use1. Ideally, there's some sort of module namespacing done, but plenty of languages manage to get away without that, and we need a flat list of items for the Rule enum anyway, unless we want to do name mangling indirection through a macro.

If it's spelled include!("base.pest") I'm less against it, since the dumb nature is more obvious.

Footnotes

  1. a uses b and c, and b and c both use d, and this doesn't result in the multiple definition of items from d.

Resolve pest-parser#197

Example:

```rust
#[derive(Parser)]
#[grammar = "base.pest"]
#[grammar = "json.pest"]
pub struct JSONParser;
```

For supports sharing rules between grammars.
Copy link
Contributor

@tomtau tomtau left a comment

Choose a reason for hiding this comment

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

lgtm, the meta grammar extension can comments can continue on #759

@tomtau tomtau merged commit 2f60362 into pest-parser:master Jan 5, 2023
huacnlee added a commit to huacnlee/pest-book that referenced this pull request Jan 9, 2023
@huacnlee huacnlee deleted the feat/use-pest branch January 9, 2023 04:00
huacnlee added a commit to huacnlee/pest-book that referenced this pull request Jan 9, 2023
tomtau added a commit to pest-parser/book that referenced this pull request Jan 29, 2023
* Add doc for load multiple pest files and `include!` syntax.

Ref:

- pest-parser/pest#759
- pest-parser/pest#758

* Apply suggestions from code review

---------

Co-authored-by: Tomas Tauber <2410580+tomtau@users.noreply.github.com>
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.

Sharing rules between grammars?
4 participants