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

nginx: Made some patterns greedy #3230

merged 1 commit into from Dec 3, 2021


Copy link

@RunDevelopment RunDevelopment commented Nov 26, 2021

No description provided.

Copy link

@github-actions github-actions bot commented Nov 26, 2021

JS File Size Changes (gzipped)

A total of 1 files have changed, with a combined diff of -3 B (-0.8%).

file master pull size diff % diff
components/prism-nginx.min.js 388 B 385 B -3 B -0.8%

Generated by 🚫 dangerJS against 004c996

JaKXz approved these changes Dec 3, 2021
Copy link

@JaKXz JaKXz commented Dec 3, 2021

This makes sense to me; do you think it's overkill to add tests for these small changes?

Copy link
Member Author

@RunDevelopment RunDevelopment commented Dec 3, 2021

do you think it's overkill to add tests for these small changes?

Yes. The greedy: true is necessary to prevent a certain kind of behavior around greedy matching (see #2680 for more details). The problem is that this behavior is really hard to reproduce, so it's really hard to find a code snippet that could be the test.

I'm actually quite sure that it's not possible to find a code snippet that behaves differently under the new grammar in this PR. For the behavior described in #2680 to occur, we need a non-greedy pattern followed by at least 2 greedy patterns that can cause rematching amongst each other. The old nginx grammar simply doesn't fulfill those conditions.

The reason why I added greedy: true here and in many other places anyway is that greedy: true is guaranteed to prevent the issue described in #2680. It's a lot easier to just say "the first pattern has to be greedy if there are other greedy patterns" and then enforce that rule with an automated test (which I plan to add eventually) than it is to apply the complex reasoning around #2680 to every language we have.

Actually, I should probably make an issue for this practice and the test that will eventually enforce it.

@RunDevelopment RunDevelopment merged commit 7b72e0a into PrismJS:master Dec 3, 2021
12 checks passed
@RunDevelopment RunDevelopment deleted the nginx-greedy branch Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants