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.

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.

2 participants