Add to support `include!` for load partial for Pest grammars. #759

huacnlee · 2023-01-04T15:51:46Z

Example

base.pest

WHITESPACE = _{ " " | "\t" | "\r" | "\n" }

json.pest

include!("base.pest")

json = { ... }

Will same as:

WHITESPACE = _{ " " | "\t" | "\r" | "\n" }

json = { ... }

huacnlee · 2023-01-04T16:08:52Z

Or how about use:

A. Import

import("base.pest")

B. Require same as Node.js, Ruby

require("base.pest")

C. Rust include! macro.

include!("base.pest")

huacnlee · 2023-01-04T16:22:11Z

@CAD97 I have updated syntax to include!("base.pest") in last commit.

tomtau

I merged that original split PR, so this can be rebased on top of the latest master

base.pest ```pest WHITESPACE = _{ " " | "\t" | "\r" | "\n" } ``` json.pest ```pest use "base.pest" json = { ... } ``` Will same as: ```pest WHITESPACE = _{ " " | "\t" | "\r" | "\n" } json = { ... } ```

huacnlee · 2023-01-05T02:12:26Z

@tomtau I have updated

tomtau

It seems all right. I don't have any special preference for the syntax: include may be fine, just that Rust-like syntax form may perhaps be confusing to lead people into thinking they can embed Rust code.

One thing that's missing is docs: when should one use this include statement versus using multiple derive expressions?

Ref: - pest-parser/pest#759 - pest-parser/pest#758

huacnlee · 2023-01-09T03:07:58Z

pest-parser/book#29

tomtau · 2023-01-09T09:53:00Z

@huacnlee from those docs, if I understand it correctly, derive and this include meta extension achieve exactly the same outcome, i.e. there is no practical difference between them, or is there (e.g. if one of them doesn't work and the other one works in some workspace or old rustc/cargo settings?)?

Ref: - pest-parser/pest#759 - pest-parser/pest#758

huacnlee · 2023-01-10T03:24:18Z

By my mind, include! just another way to let us to load partial grammars.

Because of this is same as the Rust include! macro, so it easier to understand.

In most sense, we just need focus on the .pest files (e.g. Share the .pest files to other project, we just need copy the files).

tomtau

I'm putting this one on hold for now, because it's not obvious what specific problem it solves (given multiple grammar file loading is already supported via derive generators #758) and whether it's worth extending the meta grammar with a feature that duplicates what is already possible using derive generators.

If someone from @pest-parser/maintainers @pest-parser/triage finds the extension useful in its current form and champions this PR, I think it's ok to go forward with it. Otherwise, it will be better to extend the meta grammar with a feature that is not achieved using derive generators, such as module-level scoping as sketched out in #660 .

CAD97

I've got no big concerns here. It's a simplistic form of code reuse, to be sure, but it's sufficient for simple cases and imo clear about its limitations.

Some minor impl nits and a few questions about semantics; I'll defer to @tomtau on what should block merge.

CAD97 · 2023-01-13T01:06:32Z

generator/src/lib.rs

+}
+
+/// Get path relative to `path` dir, or relative to root path
+fn partial_path(path: Option<&PathBuf>, filename: &str) -> PathBuf {


This should take Option<&Path> instead. That might just work out at the call site, or it might require &* or .as_deref(), I'm not sure.

CAD97 · 2023-01-13T01:09:34Z

generator/src/lib.rs

+fn partial_path(path: Option<&PathBuf>, filename: &str) -> PathBuf {
+    let root = match path {
+        Some(path) => path.parent().unwrap().to_path_buf(),
+        None => join_path("./"),


Since join_path looks for the presence of a file to choose between the manifest dir or source dir, this is going to always use the src dir.

We should deliberately pick one as the "location" of attribute-inline grammars, or perhaps a better choice, just disallow include!.

Threading up an error from here might be a bigger change, unfortunately.

CAD97 · 2023-01-13T01:12:09Z

generator/src/lib.rs

+    // Add .pest suffix if not exist
+    let mut filename = filename.to_string();
+    if !filename.to_lowercase().ends_with(".pest") {
+        filename.push_str(".pest");
+    }


It's possible to create file extension less files, so adding an extension on automatically is probably not a great choice. If we do want to add an extension, it should be done via conversion to and then modification via PathBuf rather than string manipulation.

CAD97 · 2023-01-13T01:12:28Z

generator/src/lib.rs

@@ -45,34 +81,16 @@ pub fn derive_parser(input: TokenStream, include_grammar: bool) -> TokenStream {

    let mut data = String::new();
    let mut path = None;
+    let mut has_use = false;


Should be has_include now.

CAD97 · 2023-01-13T01:14:49Z

generator/src/lib.rs

+    if has_use {
+        // Re-parse the data again, after replacing the `use` statement
+        pairs = match parser::parse(Rule::grammar_rules, &data) {
+            Ok(pairs) => pairs,
+            Err(error) => panic!("error parsing \n{}", error.renamed_rules(rename_meta_rule)),
+        };
+    }


Doing it this way rather than in a loop means that only a single layer of include! will be resolved; a include! of a file with an include! won't work.

At a minimum we need a test for this case. Ideally we'd make it work, but so long as it results in an error rather than silently causing corruption it's not too bad.

huacnlee · 2023-01-13T02:13:16Z

@tomtau Okey, I get it.

tomtau · 2023-01-13T05:43:34Z

@huacnlee I think one potential use case may be in contexts where one cannot use Rust derive generators. For example:

the CLI debugger loads the grammar files directly;
the online fiddle editor doesn't even have a concept of a filesystem: https://pest.rs/#editor (but I can imagine it could perhaps be extended to have "multi-tab" editing where different grammar files are pasted into different text boxes)

So in the current form, it will also need to add support for include! in pest_vm and have some sample + test in pest_debugger

* 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 <[email protected]>

huacnlee requested a review from a team as a code owner January 4, 2023 15:51

huacnlee requested review from tomtau and removed request for a team January 4, 2023 15:51

huacnlee mentioned this pull request Jan 4, 2023

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

Merged

huacnlee force-pushed the feat/use-syntax branch from 9ba7839 to 6f5ee1b Compare January 4, 2023 16:11

huacnlee force-pushed the feat/use-syntax branch 2 times, most recently from 26cf4ed to 58af2e9 Compare January 4, 2023 16:26

huacnlee changed the title ~~Add to support use for load partial for Pest grammars.~~ Add to support include! for load partial for Pest grammars. Jan 4, 2023

tomtau requested review from CAD97, NoahTheDuke and a team January 5, 2023 01:16

tomtau requested changes Jan 5, 2023

View reviewed changes

huacnlee added 2 commits January 5, 2023 10:06

Add to support use for load partial for Pest grammars.

61f2f93

base.pest ```pest WHITESPACE = _{ " " | "\t" | "\r" | "\n" } ``` json.pest ```pest use "base.pest" json = { ... } ``` Will same as: ```pest WHITESPACE = _{ " " | "\t" | "\r" | "\n" } json = { ... } ```

Update sytanx from use "base.pest" to include!("base.pest")

8b454ec

huacnlee force-pushed the feat/use-syntax branch from 58af2e9 to 8b454ec Compare January 5, 2023 02:07

tomtau reviewed Jan 5, 2023

View reviewed changes

huacnlee added a commit to huacnlee/pest-book that referenced this pull request Jan 9, 2023

Add doc for load multiple pest files and include! syntax.

0d3e966

Ref: - pest-parser/pest#759 - pest-parser/pest#758

huacnlee mentioned this pull request Jan 9, 2023

Add doc for load multiple pest files and include! syntax. pest-parser/book#29

Merged

huacnlee added a commit to huacnlee/pest-book that referenced this pull request Jan 9, 2023

Add doc for load multiple pest files and include! syntax.

0d4b43d

Ref: - pest-parser/pest#759 - pest-parser/pest#758

huacnlee requested review from tomtau and removed request for CAD97 and NoahTheDuke January 12, 2023 13:24

tomtau added on-hold needs-decision labels Jan 13, 2023

tomtau added the needs-pof label Jan 13, 2023

tomtau requested a review from CAD97 January 13, 2023 00:55

tomtau reviewed Jan 13, 2023

View reviewed changes

CAD97 requested changes Jan 13, 2023

View reviewed changes

tomtau removed on-hold needs-decision needs-pof labels Jan 13, 2023

huacnlee closed this Feb 26, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add to support `include!` for load partial for Pest grammars. #759

Add to support `include!` for load partial for Pest grammars. #759

huacnlee commented Jan 4, 2023 •

edited

Loading

huacnlee commented Jan 4, 2023

huacnlee commented Jan 4, 2023

tomtau left a comment

huacnlee commented Jan 5, 2023

tomtau left a comment

huacnlee commented Jan 9, 2023

tomtau commented Jan 9, 2023

huacnlee commented Jan 10, 2023

tomtau left a comment

CAD97 left a comment

CAD97 Jan 13, 2023

CAD97 Jan 13, 2023

CAD97 Jan 13, 2023

CAD97 Jan 13, 2023

CAD97 Jan 13, 2023

huacnlee commented Jan 13, 2023

tomtau commented Jan 13, 2023

Add to support include! for load partial for Pest grammars. #759

Add to support include! for load partial for Pest grammars. #759

Conversation

huacnlee commented Jan 4, 2023 • edited Loading

Example

huacnlee commented Jan 4, 2023

huacnlee commented Jan 4, 2023

tomtau left a comment

Choose a reason for hiding this comment

huacnlee commented Jan 5, 2023

tomtau left a comment

Choose a reason for hiding this comment

huacnlee commented Jan 9, 2023

tomtau commented Jan 9, 2023

huacnlee commented Jan 10, 2023

tomtau left a comment

Choose a reason for hiding this comment

CAD97 left a comment

Choose a reason for hiding this comment

CAD97 Jan 13, 2023

Choose a reason for hiding this comment

CAD97 Jan 13, 2023

Choose a reason for hiding this comment

CAD97 Jan 13, 2023

Choose a reason for hiding this comment

CAD97 Jan 13, 2023

Choose a reason for hiding this comment

CAD97 Jan 13, 2023

Choose a reason for hiding this comment

huacnlee commented Jan 13, 2023

tomtau commented Jan 13, 2023

Add to support `include!` for load partial for Pest grammars. #759

Add to support `include!` for load partial for Pest grammars. #759

huacnlee commented Jan 4, 2023 •

edited

Loading