Skip to content
This repository was archived by the owner on Sep 27, 2023. It is now read-only.

Conversation

@ajaaym
Copy link
Contributor

@ajaaym ajaaym commented Dec 7, 2018

Fixes #3604 Fix matcher to match path with protocol and hostname

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 7, 2018
@codecov-io
Copy link

codecov-io commented Dec 7, 2018

Codecov Report

Merging #68 into master will increase coverage by 0.19%.
The diff coverage is 75%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #68      +/-   ##
============================================
+ Coverage     60.19%   60.38%   +0.19%     
- Complexity      138      143       +5     
============================================
  Files            14       14              
  Lines           603      616      +13     
  Branches         92       94       +2     
============================================
+ Hits            363      372       +9     
- Misses          217      219       +2     
- Partials         23       25       +2
Impacted Files Coverage Δ Complexity Δ
...java/com/google/api/pathtemplate/PathTemplate.java 64.85% <75%> (+0.16%) 88 <4> (+5) ⬆️

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 e6bd3cf...8d75255. Read the comment docs.

@andreamlin
Copy link
Contributor

Thanks for filing this PR!

Does this PR address anything else other than googleapis/google-cloud-java#3604?

Regarding googleapis/google-cloud-java#3604, can you write a test that parses https://www.googleapis.com/compute/v1/projects/project-123/zones/europe-west3-c? I'm not sure if this code would be able to reconcile the compute/v1 substring between the PathTemplate string and the hostname.

(With regard to that issue, our plan is to strip off the https://www.googleapis.com/compute/v1/projects string, defined in the baseUrl of the compute.v1.json API definition file from which the Java Compute client is generated, from the returned path, so that we are left with a parseable project-123/zones/europe-west3-c substring that matches a path template {project}/zones/{zone}/, also defined in the compute.v1.json doc.)

@andreamlin andreamlin self-requested a review December 8, 2018 00:04
@ajaaym
Copy link
Contributor Author

ajaaym commented Dec 8, 2018

@andreamlin Thanks for review. Added test case for googleapis/google-cloud-java#3604. This PR was suppose to fix googleapis/google-cloud-java#3604 as well as in general to support the url with protocol and hostname. This is non breaking change would also work with your other solution.

@ajaaym
Copy link
Contributor Author

ajaaym commented Jan 4, 2019

@andreamlin can you please review?

Copy link
Contributor

@andreamlin andreamlin left a comment

Choose a reason for hiding this comment

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

LGTM

@ajaaym ajaaym merged commit 3c63521 into googleapis:master Jan 4, 2019
miraleung pushed a commit that referenced this pull request Jun 4, 2020
* Fix PathTemplate matcher to match path with protocol and hostname
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants