Skip to content

[BUGFIX] Include comments for all rules in declaration block #1169

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

Merged
merged 3 commits into from
Mar 16, 2025

Conversation

JakeQZ
Copy link
Contributor

@JakeQZ JakeQZ commented Mar 15, 2025

  • Rule::parse() will no longer consume anything after the semicolon terminating the rule - it does not belong to that rule;
  • The whitespace and comments before a rule will be processed by RuleSet::parseRuleSet() and passed as a parameter to Rule::parse() -
    • This is only required while 'strict mode' parsing is an option, to avoid having an exception thrown during normal operation (i.e. when Rule::parse() encounters normal } as opposed to some other junk, which is not distinguished).

Fixes #173.
See also #663, #672, #741.

- `Rule::parse()` will no longer consume anything after the semicolon
  terminating the rule - it does not belong to that rule;
- The whitespace and comments before a rule will be processed by
  `RuleSet::parseRuleSet()` and passed as a parameter to `Rule::parse()` -
  - This is only required while 'strict mode' parsing is an option,
    to avoid having an exception thrown during normal operation
    (i.e. when `Rule::parse()` encounters normal `}`
    as opposed to some other junk, which is not distinguished).

Fixes #173.
See also #672, #741.
@coveralls
Copy link

coveralls commented Mar 15, 2025

Coverage Status

coverage: 55.727% (-0.1%) from 55.84%
when pulling d503f75 on bugfix/ruleset-comments
into b561b72 on main.

Copy link
Contributor

@oliverklee oliverklee 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! I think we should have some more testcases, though.

Also add line-breaks in test methods to separate the three phases:
- set up;
- obtain results;
- make assertions.
@JakeQZ JakeQZ requested a review from oliverklee March 16, 2025 00:53
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Added two more nitpicks.

@oliverklee oliverklee merged commit e0e0dd3 into main Mar 16, 2025
21 checks passed
@oliverklee oliverklee deleted the bugfix/ruleset-comments branch March 16, 2025 16:25
@oliverklee
Copy link
Contributor

@JakeQZ Would you be willing to take care of creating the backport?

JakeQZ added a commit that referenced this pull request Mar 16, 2025
This is the v8.x backport of #1169.

- `Rule::parse()` will no longer consume anything after the semicolon
  terminating the rule - it does not belong to that rule;
- The whitespace and comments before a rule will be processed by
  `RuleSet::parseRuleSet()` and passed as a parameter to `Rule::parse()` -
  - This is only required while 'strict mode' parsing is an option,
    to avoid having an exception thrown during normal operation
    (i.e. when `Rule::parse()` encounters normal `}`
    as opposed to some other junk, which is not distinguished).

Fixes #173.
See also #663, #672, #741.
JakeQZ added a commit that referenced this pull request Mar 16, 2025
This is the v8.x backport of #1169.

- `Rule::parse()` will no longer consume anything after the semicolon
  terminating the rule - it does not belong to that rule;
- The whitespace and comments before a rule will be processed by
  `RuleSet::parseRuleSet()` and passed as a parameter to `Rule::parse()` -
  - This is only required while 'strict mode' parsing is an option,
    to avoid having an exception thrown during normal operation
    (i.e. when `Rule::parse()` encounters normal `}`
    as opposed to some other junk, which is not distinguished).

Fixes #173.
See also #663, #672, #741.
@JakeQZ
Copy link
Contributor Author

JakeQZ commented Mar 17, 2025

@JakeQZ Would you be willing to take care of creating the backport?

Done - #1182.

oliverklee pushed a commit that referenced this pull request Mar 17, 2025
This is the v8.x backport of #1169.

- `Rule::parse()` will no longer consume anything after the semicolon
  terminating the rule - it does not belong to that rule;
- The whitespace and comments before a rule will be processed by
  `RuleSet::parseRuleSet()` and passed as a parameter to `Rule::parse()` -
  - This is only required while 'strict mode' parsing is an option,
    to avoid having an exception thrown during normal operation
    (i.e. when `Rule::parse()` encounters normal `}`
    as opposed to some other junk, which is not distinguished).

Fixes #173.
See also #663, #672, #741.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug with parsing multiple comments
3 participants