-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Reduce duplication in badge regex/url patterns; affects [ansible apm gem wercker] #2279
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
f4a3bf8
Reduce duplication in badge regex/url patterns
paulmelnikow 870abac
Some cleanup + fixes
paulmelnikow 84ca74e
Clean diff
paulmelnikow 5ff071c
Clean diff
paulmelnikow 3a3ca25
Copyedit
paulmelnikow c43b4e2
Update GemVersion to match changes to tutorial
paulmelnikow 5f8d8fc
Give an error if `format` is missing
paulmelnikow e22c185
Fix typo in tutorial
paulmelnikow 74fbecf
Format to match tutorial
paulmelnikow a643b21
Merge branch 'master' into path-to-regexp
paulmelnikow File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think its worth mentioning the format/capture args in the docs too, or is it too much detail?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm… well I guess my hope is that we won't need to use them. Also yea, it seems like it's a bit detailed to include in such a simple example in a tutorial.
I'm curious: are there services that you think will require a hand-crafted regex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example I'd pick of one that I suspect would be quite hard:
shields/services/wercker/wercker.service.js
Line 81 in 5019d81
although I've not tried to port it (there is history to that one). Happily, these examples are in a minority.
In general, routes that use a non-capturing group might be difficult but I may wrong on that? If so, I suppose you could just capture stuff and not use it..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, that is a monster! I can't say I can follow what it's doing, even from the tests. I'm guessing it's handling some cases which aren't covered by the tests.
I imagine it's too complex to deal with using path-to-regexp with a single pattern. Though I'd wager it could be expressed clearly using two or three, two of which I imagine are deprecated.
There is support for unnamed parameters, which could be a way to handle non-capturing groups. We'd have to build a little support around those.
Though I wonder if it would be better to allow services to register multiple patterns. To eventually build the URLs from fields in the UI, it would be helpful to separate the deprecated patterns from the current pattern.
It could also make matching more efficient, down the line, because it could allow pushing things from the regex into a prefix, which could be indexed in a trie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. We're getting into the weeds a bit on an unrelated issue here, but in this specific example there is a reason why we've defined this as a regex rather than several classes:
Using the regex allows us to specify the order of matching (i.e: try to match this first, then if that fails, try to match that), whereas if we had defined one route for
ci/([a-fA-F0-9]{24})(?:/(.+))?
and one forci/([^/]+/[^/]+)(?:/(.+))?
, it matters what order we register the routes in (or what order we try to match in) because if we end up testing againstci/([^/]+/[^/]+)(?:/(.+))?
firstci/559e33c8e982fc615500b357/master
will match that, but we don't want it to.The upshot of that is if we want to allow a single badge to define multiple routes, we'd want to be able to also specify which order we'll try to match those routes in, if you see what I mean.
All of that said, lets not get too bogged down in this one case for now. As it stands, this will allow us to improve 99% of cases and it we can continue to use a regex in other situations. We can worry about these odd edge cases antoher day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, that makes sense.
I'm thinking it would be good to identify canonical forms of these routes, and use a 301 redirect. There's a small performance penalty, but it significantly increases the chance that the URLs will be updated because if they're pasted into an address bar, they will be replaced by the canonical URL. There are a couple services where we've renamed things, and there's an "old static badge" example in the bottom of
server.js
. (Honestly perhaps that last one could be deleted. I wonder if it's getting any hits.)It could be useful to set a priority for services so they can be configured to register earlier or later.
Not the highest priorities, but probably worth tackling someday.