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

feat: Add support for additional_bindings #993

Merged
merged 1 commit into from
May 19, 2022

Conversation

vam-google
Copy link
Contributor

This PR depends on the corresponding one in gax: googleapis/gax-java#1680

This PR depends on the corresponding one in gax: googleapis/gax-java#1680
@vam-google vam-google requested review from a team as code owners May 13, 2022 22:37
@sonarcloud
Copy link

sonarcloud bot commented May 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@@ -63,6 +65,8 @@ public int compareTo(HttpBinding o) {

public abstract String pattern();

public abstract List<String> additionalPatterns();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to use List here? Can we use Set? Does order matter? Do duplicates matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think technically duplicates are allowed even though they will be useless. Bindings come in list from proto file descriptor, so keeping them as list here.

@@ -22,7 +22,7 @@ public class PatternParser {

// This method tries to parse all named segments from pattern and sort in natual order
// e.g. /v1beta1/{table_name=tests/*}/{routing_id=instances/*}/** -> (routing_id, table_name)
public static Set<String> getPattenBindings(String pattern) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch! Thanks for correcting my mistake!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I actually thought it was my typo, only now realized that it was the class you created =).

@vam-google vam-google merged commit ce58c18 into googleapis:main May 19, 2022
suztomo pushed a commit that referenced this pull request Dec 16, 2022
This PR depends on the corresponding one in gax: googleapis/gax-java#1680
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release *beep* *boop*
---


### Updating meta-information for bleeding-edge SNAPSHOT release.

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants