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(routeoptimization): New code samples #9367

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bshi
Copy link

@bshi bshi commented Jun 6, 2024

Description

Fixes #

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed Sample Format Guide
  • pom.xml parent set to latest shared-configuration
  • Appropriate changes to README are included in PR
  • These samples need a new API enabled in testing projects to pass (let us know which ones)
    • routeoptimization.googleapis.com
  • [N/A] These samples need a new/updated env vars in testing projects set to pass (let us know which ones)
  • Tests pass: mvn clean verify required
  • Lint passes: mvn -P lint checkstyle:check required
  • Static Analysis: mvn -P lint clean compile pmd:cpd-check spotbugs:check advisory only
  • This sample adds a new sample directory, and I updated the CODEOWNERS file with the codeowners for this sample
  • This sample adds a new Product API, and I updated the Blunderbuss issue/PR auto-assigner with the codeowners for this sample
  • Please merge this PR for me once it is approved

@bshi bshi requested review from yoshi-approver and a team as code owners June 6, 2024 23:37
Copy link

snippet-bot bot commented Jun 6, 2024

Here is the summary of changes.

You are about to add 2 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added the samples Issues that are directly related to samples. label Jun 6, 2024
Comment on lines +53 to +57
<dependency>
<groupId>com.google.maps</groupId>
<artifactId>google-maps-routeoptimization</artifactId>
<scope>compile</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

what is a reasoning to duplicate dependency (via <dependencyManagement> and <dependencies> in this sample project?
please, use one. if you decide to use the selected, remove scope and add version number.

Copy link
Author

Choose a reason for hiding this comment

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

(NB: I'm not a java packaging expert)

AIUI this isn't a duplicate dependency, the dependencyManagement declaration is equivalent to libraries-bom declared used elsewhere (see http://shortn/_72sIg41fuQ on why libraries-bom isn't used here).

Copy link
Author

Choose a reason for hiding this comment

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

Resolving since I believe I'm not deviating from precedent modulo the differences/rationale described in the linked document.

Copy link
Contributor

Choose a reason for hiding this comment

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

The linked document is outdated (2022). It also does not reference google-maps-routeoptimization-bom. As such it cannot be used as a justification.
Unless you can clearly explain why you need both, please remove this dependency or the BOM dependency above. If you decide to remove BOM, please update this dependency by removing scope (defining compile scope is redundant since it is a default scope) and add the version of the dependency that should be used.

@bshi bshi force-pushed the main branch 2 times, most recently from a72f61f to 8efe811 Compare June 11, 2024 17:55
@bshi
Copy link
Author

bshi commented Jun 11, 2024

Thanks for the review @minherz - code updated and responses ready for another look.

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/blunderbuss.yml Outdated Show resolved Hide resolved
@iennae iennae requested a review from a team as a code owner June 22, 2024 05:00
@minherz minherz self-requested a review June 24, 2024 15:08
Copy link
Contributor

@minherz minherz left a comment

Choose a reason for hiding this comment

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

Please, resolve the duplicated dependency declaration.

<groupId>com.google.maps</groupId>
<scope>import</scope>
<type>pom</type>
<version>0.1.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

please use the latest (0.3.0) version.

Comment on lines +53 to +57
<dependency>
<groupId>com.google.maps</groupId>
<artifactId>google-maps-routeoptimization</artifactId>
<scope>compile</scope>
</dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

The linked document is outdated (2022). It also does not reference google-maps-routeoptimization-bom. As such it cannot be used as a justification.
Unless you can clearly explain why you need both, please remove this dependency or the BOM dependency above. If you decide to remove BOM, please update this dependency by removing scope (defining compile scope is redundant since it is a default scope) and add the version of the dependency that should be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants