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

fix: fix match logic for path template #834

Merged
merged 4 commits into from May 28, 2020
Merged

Conversation

xiaozhenliu-gg5
Copy link
Contributor

failing case: match projects/{project=*} with project: long-project-name from dialogflow sample-test
googleapis/nodejs-dialogflow#617

the original logic is to check the matching string (long-project-name) if it can be divided by -_.~, to decide whether it's non-slash resource (for example match {user_a}-{user_b} with usera-userb). but it will fail for the above case.
Now we check the segments {project=*} if it has multiple brackets, it will be taken as non-slash resource (for example {user_a}-{user_b}

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 27, 2020
@codecov
Copy link

codecov bot commented May 27, 2020

Codecov Report

Merging #834 into master will increase coverage by 29.28%.
The diff coverage is 82.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #834       +/-   ##
===========================================
+ Coverage   59.68%   88.96%   +29.28%     
===========================================
  Files          46       38        -8     
  Lines        7426     6344     -1082     
  Branches      107      508      +401     
===========================================
+ Hits         4432     5644     +1212     
+ Misses       2994      697     -2297     
- Partials        0        3        +3     
Impacted Files Coverage Δ
src/pathTemplate.ts 89.65% <82.14%> (+59.45%) ⬆️
tools/prepublish.ts 0.00% <0.00%> (-100.00%) ⬇️
src/normalCalls/timeout.ts 98.21% <0.00%> (ø)
samples/pagination.js
...ixtures/google-gax-packaging-test-app/src/index.js
.prettierrc.js
webpack.config.js
...-gax-packaging-test-app/src/v1beta1/echo_client.js
...google-gax-packaging-test-app/src/v1beta1/index.js
samples/quickstart.js
... and 31 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4449426...4cebe2e. Read the comment docs.

@@ -35,6 +35,8 @@ export class PathTemplate {
constructor(data: string) {
this.data = data;
this.segments = this.parsePathTemplate(data);
console.warn('this.bindings: ', this.bindings);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the logging output

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! done.

Copy link
Member

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after logging is removed, thank you!

@xiaozhenliu-gg5 xiaozhenliu-gg5 merged commit d0d5782 into master May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants