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

Improved docs on map matching. #102

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nisbet-hubbard
Copy link

This PR improves the wording regarding map testing to reflect the performance benefits of using exact match over regex.

https://forum.nginx.org/read.php?2,214693,214694

@y82
Copy link
Collaborator

y82 commented Nov 6, 2024

Hi @nisbet-hubbard,

Thank you for your contribution and for your attention to detail. I still find the existing phrasing — “regular expression match” and “matching variant chosen” — quite clear and consistent with our current terminology. Additionally, we haven’t previously used the term “testing” in this context, while phrases like “strings evaluated/matched” and “regular expressions matched” are more established.

This is just my perspective, and other community members may have different thoughts on this.

@y82 y82 self-assigned this Nov 6, 2024
@nisbet-hubbard
Copy link
Author

Thanks! I’ve updated the patch to address these concerns.

What bothers me in the original version is that wording like ‘If the source value matches more than one of the specified variants’ doesn’t make it clear (to people who are not already familiar with the codebase) whether all the variants are actually searched or just the variants up to the first match. So, I’m borrowing a phrase from the docs on the location directive to clarify this point.

@nisbet-hubbard nisbet-hubbard changed the title Improve docs on map testing Improved docs on map matching. Nov 18, 2024
@nisbet-hubbard
Copy link
Author

Rebased as per guidelines.

@pluknet
Copy link
Collaborator

pluknet commented Jan 10, 2025

In my personal reading of the current wording:
"the first matching variant will be chosen"
already implies that
"the search terminates on the first match".

Further, regarding the patch, I don't like that "the first matching" is used twice, it worsens the wording quality.

@nisbet-hubbard
Copy link
Author

Absolutely, and it's because you already know that's what it means.

What I was trying to get across is we understand what the current wording implies on personal reading because we already know what it implies. And we need to stand in the shoes of someone who doesn't.

In any case, here's an alternative edit:

If the source value potentially matches more than one of the specified variants

Would this satisfy everyone?

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.

3 participants