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: enable complex resource ids in instantiate() #159

Merged
merged 1 commit into from Jul 1, 2020
Merged

Conversation

miraleung
Copy link
Collaborator

@miraleung miraleung commented Jun 30, 2020

Fixes #154.

@miraleung miraleung requested a review from vam-google Jun 30, 2020
@googlebot googlebot added the cla: yes label Jun 30, 2020
@codecov
Copy link

@codecov codecov bot commented Jun 30, 2020

Codecov Report

Merging #159 into master will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #159      +/-   ##
============================================
+ Coverage     64.09%   64.43%   +0.33%     
- Complexity      171      174       +3     
============================================
  Files            14       14              
  Lines           713      717       +4     
  Branches        118      120       +2     
============================================
+ Hits            457      462       +5     
+ Misses          220      219       -1     
  Partials         36       36              
Impacted Files Coverage Δ Complexity Δ
...java/com/google/api/pathtemplate/PathTemplate.java 69.66% <100.00%> (+0.50%) 117.00 <0.00> (+3.00)

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 39612f1...a178f1a. Read the comment docs.

Copy link
Collaborator

@vam-google vam-google left a comment

LGTM

@devchas
Copy link

@devchas devchas commented Jul 1, 2020

I just tested this out with a local build, and it seems to have fixed the problem. Thank you for the quick fix here! Any idea when this might make it into the next release. As mentioned in #154, we have another large body of work which depends on this feature, with a hard deadline in mid-July.

@miraleung miraleung merged commit 955b8a7 into master Jul 1, 2020
5 checks passed
@miraleung miraleung deleted the fix/instantiate branch Jul 1, 2020
Truth.assertThat(match.get("foo")).isEqualTo("foo1");
Truth.assertThat(match.get("bar")).isEqualTo("bar2");
variables = template.vars();
System.out.println("DEL: vars: " + variables);
Copy link
Contributor

@elharo elharo Jul 1, 2020

Choose a reason for hiding this comment

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

This shouldn't be here

.containsExactly("foo", "bar", "zone_a", "zone_b", "zone_c", "cell1", "cell2");

pattern = "projects/{foo}_{bar}/zones/*";
template = PathTemplate.create(pattern);
Copy link
Contributor

@elharo elharo Jul 1, 2020

Choose a reason for hiding this comment

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

local variables probably shouldn't be reused

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants