8000 Updated comments in rule files on how to use regexp-assemble.py by theseion · Pull Request #2423 · coreruleset/coreruleset · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Updated comments in rule files on how to use regexp-assemble.py #2423

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 10, 2022
Merged

Updated comments in rule files on how to use regexp-assemble.py #2423

merged 4 commits into from
Apr 10, 2022

Conversation

theseion
Copy link
Contributor
@theseion theseion commented Mar 6, 2022

This PR accompanies #2422.

@theseion theseion requested review from franbuehler and airween March 6, 2022 16:43
@airween
Copy link
Contributor
airween commented Mar 6, 2022

I hope you will continue to unify the regexp-assembly script documentation :).

Now I'm using this regex to catch the build instruction in the CRS Rules-doc script:

        self.re_buildinst = re.compile("To rebuild the (regexp|regular expression)")

It would be good to see that each description uses the same syntax. Thanks :).

RedXanadu
RedXanadu reviewed Mar 6, 2022
View reviewed changes
RedXanadu
RedXanadu reviewed Mar 6, 2022
View reviewed changes
@theseion
Copy link
Contributor Author
theseion commented Mar 6, 2022

Thanks guys! I've changed the comment a bit but I think I got them all this time.

I don't think there's any merit in having "word list" in two places, it doesn't add any useful information, IMO.

@dune73 dune73 mentioned this pull request Mar 7, 2022
Closed
@franbuehler
Copy link
Contributor

Meeting decision March 7: @RedXanadu already reviewed. @franbuehler and @airween are added as reviewers.

airween
airween approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor
@airween airween left a comment

Choose a reason for hiding this comment

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

I reviewed this PR, tried with the parser of the CRS documentation (which is also aligned).

Looks good to me.

@fzipi fzipi mentioned this pull request Apr 4, 2022
Closed
@dune73
Copy link
Member
dune73 commented Apr 8, 2022

We discussed this in the previous meeting:

#2423 depends on #2422 that still needs a review by @franbuehler and @airween.

Adding do-not-merge label.

@dune73 dune73 added the ⚠️ do not merge Additional work or discussion is needed despite passing tests label Apr 8, 2022
@dune73 dune73 added this to the CRS v4.0.0 milestone Apr 8, 2022
@dune73 dune73 mentioned this pull request Apr 8, 2022
Merged
@theseion theseion force-pushed the unified-regex-utils-update-comments branch 2 times, most recently from bd76aba to 782ece0 Compare April 9, 2022 16:04
@theseion theseion force-pushed the unified-regex-utils-update-comments branch from 782ece0 to 9553cf4 Compare April 9, 2022 16:14
@dune73
Copy link
Member
dune73 commented Apr 10, 2022

Removing the do-not-merge label since #2422 has been merged in the meantime.

The merging of this PR is critical for the v.4-RC.

@dune73 dune73 removed the ⚠️ do not merge Additional work or discussion is needed despite passing tests label Apr 10, 2022
@airween
Copy link
Contributor
airween commented Apr 10, 2022

Removing the do-not-merge label since #2422 has been merged in the meantime.

The script which builds the CRSDoc structure uses heavily these comments. Let me check the modifications with that before the merge.

@airween airween self-assigned this Apr 10, 2022
@dune73
Copy link
Member
dune73 commented Apr 10, 2022

Very good. Thanks.

@theseion
Copy link
Contributor Author

Removing the do-not-merge label since #2422 has been merged in the meantime.

The script which builds the CRSDoc structure uses heavily these comments. Let me check the modifications with that before the merge.

Thanks, I wasn't aware of that.

@airween
Copy link
Contributor
airween commented Apr 10, 2022

The modified rules are fine.

But I found few rules where the build instructions are in some old format (what I don't handle either in the crsdoc script).

@theseion do you think you can change these lines too?

932300
932310
932320
932301
932311
932321

Thanks!

@theseion
Copy link
Contributor Author

Thanks @airween, nicely spotted. I also found another lingering one (934120). I hope I got them all now.

@theseion theseion merged commit 43fe04f into coreruleset:v3.4/dev Apr 10, 2022
@theseion theseion deleted the unified-regex-utils-update-comments branch April 10, 2022 15:23
@airween
Copy link
Contributor
airween commented Apr 10, 2022

I hope I got them all now.

Sorry, after I grabbed you commit, I found a new one occurrence:

930100.

@theseion
Copy link
Contributor Author

That one has to stay that way for now. There's no good way at the moment to do it differently. I'm working on some enhancements to regexp-assemble that will take care of that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RedXanadu RedXanadu RedXanadu left review comments

@airween airween airween approved these changes

@franbuehler franbuehler Awaiting requested review from franbuehler

Assignees

@airween airween

Labels
None yet
Projects
None yet
Milestone
CRS v4.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@theseion @airween @franbuehler @dune73 @RedXanadu @fzipi
0