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: resource name class deduplication #1854

Merged
merged 10 commits into from
Jul 18, 2023
Merged

Conversation

diegomarquezp
Copy link
Contributor

@diegomarquezp diegomarquezp commented Jul 17, 2023

Adds deduplication logic for com.google.api.resourcenames.ResourceName implementing classes. It is possible that a generated class is also named ResourceName, which is not currently handled by the generator.

This approach consists of:

  • modifying ResourceNameClassComposer to use the full name of ResourceName (the only implemented interface) if there is a name collision with the class being generated

@diegomarquezp diegomarquezp changed the title Resource name deduplication feat: resource name class deduplication Jul 17, 2023
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Jul 17, 2023
@diegomarquezp diegomarquezp requested a review from blakeli0 July 17, 2023 17:15
@diegomarquezp diegomarquezp marked this pull request as ready for review July 17, 2023 17:53
@diegomarquezp diegomarquezp requested a review from a team as a code owner July 17, 2023 17:53
* @return
*/
private static List<TypeNode> createImplementsTypes(String implementingClassName) {
List<TypeNode> preprocessed = Arrays.asList(FIXED_TYPESTORE.get("ResourceName"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

FIXED_TYPESTORE always returns only one result. Can we check if there is conflict and fix it first, then wrap it as a list in the end?

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 used a slightly different but similarly concise way (wrap both possible results in the end). Hope you agree it's good too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, this looks much better!
nit: It would be great if we can return TypeNode only in this private method and wrap it as a list when we call it, since the list always contains just one element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I only saw the review comment instead of this one. I'll leave it as a quick follow up

@diegomarquezp diegomarquezp requested a review from blakeli0 July 18, 2023 01:24
@sonarqubecloud
Copy link

[gapic-generator-java-root] 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

@sonarqubecloud
Copy link

[java_showcase_integration_tests] 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

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarqubecloud
Copy link

[java_showcase_unit_tests] 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

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

Thanks Diego! Looking good to me other than one nit. We should update the description as it does not reflect the latest changes. Also, this PR should probably be a fix instead of feat.

@diegomarquezp diegomarquezp changed the title feat: resource name class deduplication fix: resource name class deduplication Jul 18, 2023
@diegomarquezp diegomarquezp merged commit 08eca7d into main Jul 18, 2023
@diegomarquezp diegomarquezp deleted the resource-name-deduplication branch July 18, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants