feat: parse path template using regexes#823
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #823 +/- ##
==========================================
- Coverage 90.05% 89.63% -0.43%
==========================================
Files 48 46 -2
Lines 8134 7408 -726
Branches 493 549 +56
==========================================
- Hits 7325 6640 -685
+ Misses 806 765 -41
Partials 3 3
Continue to review full report at Codecov.
|
JustinBeckwith
left a comment
JustinBeckwith
left a comment
There was a problem hiding this comment.
Looks fine to me. Can we also drop the pegjs dependency?
| } else { | ||
| out.push(segment); | ||
| if (Object.keys(bindings).length !== Object.keys(this.bindings).length) { | ||
| throw new TypeError('The number of variables does not match'); |
There was a problem hiding this comment.
This error message is not actionable. The previous one, 'Value for key %s is not provided in %s', gives some clue about what's wrong. Can we improve it here?
Please note that all the errors thrown from here will be user visible. Expect to receive a client library issue for all the error messages that are just a little bit unclear :)
There was a problem hiding this comment.
sure, here it will complain if the given bindings do not have the same variable number with the needed variables.
This sounds better maybe:
`The number of variables ${
Object.keys(bindings).length
} does not match the number of needed variables ${
Object.keys(this.bindings).length
}`
| // if the path contains a wildcard, then the length may differ by 1. | ||
| if (!this.data.includes('**')) { | ||
| throw new TypeError( | ||
| `This path ${path} does not match, the parameter number is not same.` |
There was a problem hiding this comment.
This path does not match what exactly? ("match" is something that has 2 parameters, "a matches b", and it makes sense to have both in the error message)
There was a problem hiding this comment.
reasonable, i have include this.data (original path template) in the error messages.
| } | ||
| // {project} / {project=*} -> segments.push('{project=*}'); | ||
| // -> bindings['project'] = '*' | ||
| else if (segment.match(/(?<={)[0-9a-zA-Z-.~_]+(=\*)?(?=})/)) { |
There was a problem hiding this comment.
Look behinds are nice! :)
I will need some time to review this logic, please don't merge just yet.
There was a problem hiding this comment.
sounds great! Appreciate that!
| throw new TypeError(msg); | ||
| } else if (this.segments[index].includes('*')) { | ||
| const segment = this.segments[index]; | ||
| const variable = segment.match(/(?<={)[$0-9a-zA-Z_]+(?==.*})/g); |
There was a problem hiding this comment.
Just in case, please use minimal quantifier here (.*? instead of .*). In an (invalid) case of a path template having two =......} occurrences, we want to make sure .* eats the minimum possible match, hence .*?.
There was a problem hiding this comment.
Thanks! applying the changes now.
This PR is only for rewriting the logic, and make sure all original unit tests pass. I will make a separate one to add non-slash-resource logic.
alexander-fenster
left a comment
alexander-fenster
left a comment
There was a problem hiding this comment.
Let's try that :)
🤖 I have created a release \*beep\* \*boop\* --- ## [2.4.0](https://www.github.com/googleapis/gax-nodejs/compare/v2.3.1...v2.4.0) (2020-05-21) ### Features * parse path template using regexes ([#823](https://www.github.com/googleapis/gax-nodejs/issues/823)) ([392a392](https://www.github.com/googleapis/gax-nodejs/commit/392a3920df6d78981ac43741f15048c84102b046)) * support non-slash resource in path template ([#833](https://www.github.com/googleapis/gax-nodejs/issues/833)) ([76696fc](https://www.github.com/googleapis/gax-nodejs/commit/76696fc48c8a5e21c3c1cde56822b7a37585e41c)) ### Bug Fixes * new typescript, strict types ([#824](https://www.github.com/googleapis/gax-nodejs/issues/824)) ([90034ce](https://www.github.com/googleapis/gax-nodejs/commit/90034ce6a8c9b635942fedb23345105264979416)) * typescript 3.9.3 compilation ([#831](https://www.github.com/googleapis/gax-nodejs/issues/831)) ([d53e169](https://www.github.com/googleapis/gax-nodejs/commit/d53e16988aa0fa260c91f84e6fc0ceae2fdecc26)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
4 participants
pegjsgrammar.pegjsgeneratedpathTemplateParser.jsparserExtras.ts.pegjsrelated unit tests.src/*.jsfor compile in package.jsonPathTemplateclass has same functionality as before, but thesize/segmentshave been changed their meanings (not sure what they do originally, we don't use them anywhere, but removing them will be a breaking change).