plugins: replace most verbose matcher regexes #6285
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Split up verbose pluginmatcher regexes into multiple matchers with non-verbose regexes, or translate unnecessary verbose regexes into non-verbose ones, but keep verbose pluginmatcher regexes where it makes sense - Update plugin logic and refer to the matcher name instead of the capture group name where it makes sense - Optimize regexes in certain cases, based on URL data from tests - Update tests accordingly
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The motivation for this change is the addition of a
--show-pluginmatchers/--show-matchersCLI argument, which could list the matchers of a specific plugin (or all plugins), which allows users to check why an input URL might not have matched.As mentioned in the commit message body, I rewrote some regexes, so there's potential for mistakes here. Albavision is one of those plugins, whose regex tries to match lots of hostnames and then all potential URL paths accross all sites.
In regards to capture group names, those were not changed, so no plugin logic had to be touched. One exception is the tf1 plugin, which now refers to the name of the matched pluginmatcher instead of the capture group. This probably could've been done in other plugins too, but I didn't want to spend any time on this.
Yes, adding more plugin matchers instead of having just one big verbose regex is bad performance-wise, but this is totally insignificant. Verbose "catch-all" regexes are bad legibility/maintainability-wise and are bad for a CLI arg as mentioned above.
I will have another look at the diff later before merging...