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 /// and //! syntax for add doc comment for rules. #765

Merged
merged 5 commits into from Jan 18, 2023

Conversation

huacnlee
Copy link
Member

@huacnlee huacnlee commented Jan 9, 2023

Resolve #748

For example:

//! A parser for JSON file.

/// Matches object, e.g.: `{ "foo": "bar" }`
object = { "{" ~ pair ~ ("," ~ pair)* ~ "}" | "{" ~ "}" }

should generate:

/// A parser for JSON file.
enum Rule {
    /// Matches object, e.g.: `{ "foo": "bar" }`
    object,
}

SCR-20230109-p3j

@huacnlee huacnlee requested a review from a team as a code owner January 9, 2023 12:03
@huacnlee huacnlee requested review from tomtau and removed request for a team January 9, 2023 12:03
@tomtau tomtau added the blocked label Jan 9, 2023
@tomtau tomtau added this to the v3.0 milestone Jan 9, 2023
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! sadly, it looks like it's a semver breaking change:

Checking pest_meta
    Checking <unknown> v2.5.0 -> v2.5.2 (patch change)
   Completed [   0.141s] 33 checks; 31 passed, 2 failed, 0 unnecessary

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/constructible_struct_adds_field.ron

Failed in:
  field Rule.comments in meta/src/ast.rs:22
  field ParserRule.comments in meta/src/parser.rs:55
  field OptimizedRule.comments in meta/src/optimizer/mod.rs:105

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-check/tree/v0.15.2/src/queries/enum_variant_added.ron

Failed in:
  variant Expr:LineDoc in meta/src/ast.rs:93
  variant ParserExpr:LineDoc in meta/src/parser.rs:171
  variant OptimizedExpr:LineDoc in meta/src/optimizer/mod.rs:140
       Final [   0.161s] semver requires new major version: 2 major and 0 minor checks failed

not sure if it's possible to do in a non-breaking way (e.g. under a feature flag)... maybe mark that enum #[non_exhaustive] and make that comments field private (instead of public)?

meta/src/parser.rs Show resolved Hide resolved
meta/src/parser.rs Outdated Show resolved Hide resolved
@huacnlee
Copy link
Member Author

huacnlee commented Jan 9, 2023

Looks like we can't do it without the breaking changes.

We must add comments fields into Rule, ParserRule and OptimizedRule struct for storage the parsed comment strings. There no other way to store them.


Expr::LineDoc, ParserExpr::LineDoc and OptimizedExpr::LineDoc I have removed, it not need.

}
}

pub(crate) fn generate(
Copy link
Member Author

Choose a reason for hiding this comment

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

This because of themod generator is not public.

So I changed it to pub(crate).

@huacnlee
Copy link
Member Author

huacnlee commented Jan 9, 2023

@tomtau I have rewrite the implementation without make breaking changes.

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.

awesome work if it was made non-semver breaking! I'll have a look at in more detail hopefully soon

@@ -7,11 +7,12 @@
// option. All files in the project carrying such notice may not be copied,
// modified, or distributed except according to those terms.

grammar_rules = _{ SOI ~ grammar_rule+ ~ EOI }
grammar_rules = _{ SOI ~ grammar_doc* ~ (grammar_rule)+ ~ EOI }

grammar_rule = {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it'll be good to "dogfood" this change in the meta grammar and use it to add doc comments, but that could be done in a follow up

Copy link
Contributor

Choose a reason for hiding this comment

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

I could possibly do that in a follow up PR

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.

the non-breaking version seems all right, but if possible, could DocComment hide its internal fields (for a better "encapsulation") ?

generator/src/generator.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
@tomtau tomtau removed this from the v3.0 milestone Jan 18, 2023
@tomtau
Copy link
Contributor

tomtau commented Jan 18, 2023

#770 was merged -- could you rebase on top of the latest master?

Resolve pest-parser#748

For example:

```rust
//! A parser for JSON file.

/// Matches object, e.g.: `{ "foo": "bar" }`
object = { "{" ~ pair ~ ("," ~ pair)* ~ "}" | "{" ~ "}" }
```

should generate:

```rust
/// A parser for JSON file.
enum Rule {
    /// Matches object, e.g.: `{ "foo": "bar" }`
    object,
}
```
@huacnlee huacnlee force-pushed the feat/doc-comments branch 2 times, most recently from 8bb2e3a to 8fba851 Compare January 18, 2023 08:20
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, just a small nit about the string construction, but ok to merge

generator/src/lib.rs Outdated Show resolved Hide resolved
generator/src/generator.rs Outdated Show resolved Hide resolved
generator/src/lib.rs Outdated Show resolved Hide resolved
@huacnlee huacnlee force-pushed the feat/doc-comments branch 2 times, most recently from 63410d2 to 0cd5af4 Compare January 18, 2023 11:05
@tomtau tomtau merged commit 7bd2095 into pest-parser:master Jan 18, 2023
@huacnlee huacnlee deleted the feat/doc-comments branch January 18, 2023 12:00
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.

Request: Insert doc comments from .pest file to the Rule struct.
2 participants