Reduce duplication in badge regex/url patterns; affects [ansible apm gem wercker] #2279
Conversation
This reduces duplication in badge regex/url patterns, and reduce the need to understand regexes in order to create badges. The badge examples could be harmonized to reduce the duplication even further, though that could be handled in a follow-on PR. Ref: #2050
Generated by 🚫 dangerJS |
| @@ -320,7 +368,7 @@ describe('BaseService', function() { | |||
| }) | |||
|
|
|||
| describe('ScoutCamp integration', function() { | |||
| const expectedRouteRegex = /^\/foo\/([^/]+).(svg|png|gif|jpg|json)$/ | |||
| const expectedRouteRegex = /^\/foo\/([^/]+?)\.(svg|png|gif|jpg|json)$/ | |||
There was a problem hiding this comment.
- Today I learned that
+?is a lazy qualifier, which causes the segment to match as little as possible. It doesn't make this any less correct, and I imagine makes it easier to build pattern regexes more reliably. .to\.fixes a minor bug. Before this change, we accept any character as the extension separator.
chris48s
changed the title
There was a problem hiding this comment.
I am on-board with this 👍 A few comments inline, but I like the way we can use this for the simple cases (most routes) but fall back to regex if we need to do something a bit non-standard.
Also I've added [ansible apm wercker] to the PR title so we should run the tests for services you're touching and also making sure we don't regress with a more complex example next time you push to the branch
| This declaration adds the route `/^\/test\/([^\/]+)\.(svg|png|gif|jpg|json)$/` to our application. | ||
| * `pattern` defines the variable part of the route. It can include any | ||
| number of named parameters. These are converted into | ||
| regular expressions by [`path-to-regexp`][path-to-regexp]. |
There was a problem hiding this comment.
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.
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.
I'm curious: are there services that you think will require a hand-crafted regex?
This is an example I'd pick of one that I suspect would be quite hard:
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.
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.
Though I'd wager it could be expressed clearly using two or three, two of which I imagine are deprecated
Though I wonder if it would be better to allow services to register multiple patterns.
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 for ci/([^/]+/[^/]+)(?:/(.+))?, it matters what order we register the routes in (or what order we try to match in) because if we end up testing against ci/([^/]+/[^/]+)(?:/(.+))? first ci/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.
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.
chris48s
changed the title
|
For a small change this is pretty high impact. While I'd love to give the other maintainers a chance to weigh in, I'm also eager to land it so we can start using it, and also so we can push forward on the next step which would be reducing some duplication between the route and the examples. Hopefully everyone will be cool with this change – though if not I'm certainly happy to continue the discussion! 😉 |
|
The new approach is fine by me. 👍 |
|
Nice work! 👍 |
This continues the work from #2279, by allowing example badges to be specified using `namedParams`. Using an object makes it possible for us to display these in form fields down the line. (#701) I've called this the "preferred" way, and labeled the other ways deprecated. I've also added some doc to the `examples` property in BaseService. Then I realized we had some doc in the tutorial, though I think it's fine to have a short version in the tutorial, and the gory detail in BaseService. I've also added a `pattern` keyword, and made `urlPattern` an alias. Closes #2050.
This reduces duplication in badge regex/url patterns, and reduces the need to understand regexes in order to create badges.
The badge examples could be harmonized to reduce the duplication even further, though that could be handled in a follow-on PR.
Ref: #2050