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
merged 1 commit into from Dec 3, 2021

Conversation

RunDevelopment
Copy link
Member

No description provided.

@github-actions
Copy link

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
Copy link
Collaborator

JaKXz commented Dec 3, 2021

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

@RunDevelopment
Copy link
Member Author

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
@RunDevelopment RunDevelopment deleted the nginx-greedy branch December 3, 2021 12:58
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.

None yet

2 participants