Skip to content

Automl translation ga #1614

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Oct 17, 2019
Merged

Automl translation ga #1614

merged 12 commits into from
Oct 17, 2019

Conversation

nnegrey
Copy link
Contributor

@nnegrey nnegrey commented Oct 14, 2019

No description provided.

@nnegrey nnegrey requested a review from a team October 14, 2019 20:23
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2019
@lesv
Copy link
Contributor

lesv commented Oct 14, 2019

Java 8 is failing.

lesv
lesv approved these changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

LGTM - Nits that annoy only me, but do at least read them.

translate/automl/src/test/java/com/google/cloud/translate/automl/DatasetIT.java Outdated
datasetId =
bout.toString()
.split("\n")[0]
.split("/")[(bout.toString().split("\n")[0]).split("/").length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be using got instead? perhaps using an intermediate form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yep.
Simplified to: datasetId = got.split("Dataset id: ")[1].split("\n")[0];

translate/automl/src/test/java/com/google/cloud/translate/automl/ModelIT.java Outdated
// LIST MODELS
ListModels.listModels(PROJECT_ID);
String got = bout.toString();
modelId = got.split("\n")[1].split("/")[got.split("\n")[1].split("/").length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but shouldn't this be more efficient on 2 lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Realized I can simplify it to: modelId = got.split("Model id: ")[1].split("\n")[0];

translate/automl/src/main/java/com/google/cloud/translate/automl/ListModels.java Outdated
// Display the model information.
System.out.format("Model name: %s\n", model.getName());
System.out.format(
"Model id: %s\n", model.getName().split("/")[model.getName().split("/").length - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should use an intermediate variable?

translate/automl/src/main/java/com/google/cloud/translate/automl/GetOperationStatus.java Outdated
System.out.println("\tMetadata:");
System.out.format("\t\tType Url: %s\n", operation.getMetadata().getTypeUrl());
System.out.format(
"\t\tValue: %s\n", operation.getMetadata().getValue().toStringUtf8().replace("\n", ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but could you clean this up a bit and teach something here?

translate/automl/src/main/java/com/google/cloud/translate/automl/GetOperationStatus.java Outdated
System.out.println("\tResponse:");
System.out.format("\t\tType Url: %s\n", operation.getResponse().getTypeUrl());
System.out.format(
"\t\tValue: %s\n", operation.getResponse().getValue().toStringUtf8().replace("\n", ""));
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but could you clean this up a bit and teach something here?

translate/automl/src/main/java/com/google/cloud/translate/automl/GetModel.java Outdated
// Display the model information.
System.out.format("Model name: %s\n", model.getName());
System.out.format(
"Model id: %s\n", model.getName().split("/")[model.getName().split("/").length - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but could you clean this up a bit and teach something here?

translate/automl/src/main/java/com/google/cloud/translate/automl/GetDataset.java Outdated
System.out.format("Dataset name: %s\n", dataset.getName());
System.out.format(
"Dataset id: %s\n",
dataset.getName().split("/")[dataset.getName().split("/").length - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but could you clean this up a bit and teach something here? Why do I see the same code in two places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The create, get, list for both datasets and models are all very similar.

translate/automl/src/main/java/com/google/cloud/translate/automl/CreateDataset.java Outdated
System.out.format("Dataset name: %s\n", createdDataset.getName());
System.out.format(
"Dataset id: %s\n",
createdDataset.getName().split("/")[createdDataset.getName().split("/").length - 1]);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, but could you clean this up a bit and teach something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would something like this be good?

      // To get the dataset id, you have to parse it out of the `name` field. As dataset Ids are
      // required for other methods.
      // Name Form: `projects/{project_id}/locations/{location_id}/datasets/{dataset_id}`
      int lastIndex = createdDataset.getName().split("/").length - 1;
      String datasetId = createdDataset.getName().split("/")[lastIndex];
      System.out.format("Dataset id: %s\n", datasetId);

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking more like:

String[] names = createdDataset.getName().split("/");
String datasetId = names[names.length - 1];

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 will probably keep the comment in there too.

lesv
lesv reviewed Oct 16, 2019
View reviewed changes
translate/automl/src/main/java/com/google/cloud/translate/automl/ListDatasets.java
// required for other methods.
// Name Form: `projects/{project_id}/locations/{location_id}/datasets/{dataset_id}`
String[] names = dataset.getName().split("/");
String retrievedDatasetId = names[names.length - 1];
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks -- I'm happy w/ this, though I ran across https://stackoverflow.com/a/15317034/738710 which sadly has a better answer. No need to use it, but for the future:
String retrievedDatasetId = dataset.getName().substring( sentence.lastIndexOf("/") + 1);

@lesv
Copy link
Contributor

lesv commented Oct 16, 2019

BTW - Java 8 is failing.

@nnegrey
Copy link
Contributor Author

nnegrey commented Oct 16, 2019

Ah, I see why. That's the old beta files.
I have a PR to fix those tests: #1606

@nnegrey nnegrey merged commit 6423b40 into master Oct 17, 2019
@nnegrey
Copy link
Contributor Author

nnegrey commented Oct 17, 2019 •
edited
Loading

DO_NOT_DELETE BRANCH
TODO: Update docs samples to master branch
TODO: Move samples to refactored location

@nnegrey
Copy link
Contributor Author

nnegrey commented Oct 28, 2019

Files moved to: #1621

@kurtisvg kurtisvg deleted the automl-translation-ga branch November 15, 2019 21:36
bradmiro pushed a commit that referenced this pull request Dec 2, 2019
@nnegrey @bradmiro
Automl translation ga (#1614)
89435fa 
* Convert samples to new style guides and update to GA. Add missing samples

* Update samples and tests

* Update ExportDataset.java

* Update Prediction.java

* Update based on feedback

* Update ListOperationStatus.java

* Update ID of test dataset

* Lint: Update License header and import order
Shabirmean pushed a commit that referenced this pull request Nov 17, 2022
@nnegrey @chingor13
samples: Automl translation ga (#1614)
6ec057a 
* Convert samples to new style guides and update to GA. Add missing samples

* Update samples and tests

* Update ExportDataset.java

* Update Prediction.java

* Update based on feedback

* Update ListOperationStatus.java

* Update ID of test dataset

* Lint: Update License header and import order
@Shabirmean Shabirmean mentioned this pull request Nov 17, 2022
Merged
9 tasks
Shabirmean pushed a commit that referenced this pull request Nov 18, 2022
@nnegrey @Shabirmean
samples: Automl translation ga (#1614)
7309ca3 
* Convert samples to new style guides and update to GA. Add missing samples

* Update samples and tests

* Update ExportDataset.java

* Update Prediction.java

* Update based on feedback

* Update ListOperationStatus.java

* Update ID of test dataset

* Lint: Update License header and import order
anguillanneuf pushed a commit that referenced this pull request Dec 5, 2022
@nnegrey @anguillanneuf
samples: Automl translation ga (#1614)
cd067c8 
* Convert samples to new style guides and update to GA. Add missing samples

* Update samples and tests

* Update ExportDataset.java

* Update Prediction.java

* Update based on feedback

* Update ListOperationStatus.java

* Update ID of test dataset

* Lint: Update License header and import order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lesv lesv lesv approved these changes

Assignees

@lesv lesv

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nnegrey @lesv @googlebot