Codegen/discovery #791

germanattanasio · 2017-09-05T20:49:50Z

Generated code for discovery
Query method

This PR adds models for the various QueryAggregation types. Tests have been updated accordingly.

The new Training Data endpoint is tested. Some models were also changed to make tests pass, as they were generated wrong. This issue has since been fixed.

Add unit and integration tests

Update codegen/discovery with the latest changes from develop

mkistler

Looks like there are still issues with consistent formatting. But other than that things all look good.

mkistler · 2017-09-06T21:51:54Z

discovery/src/main/java/com/ibm/watson/developer_cloud/discovery/v1/model/Collection.java

 */
 public interface Status {
- /** active. */
+ /**


More funky reformatting. Did this come from the eclipse formatter?

Yeah I think it did.

codecov-io · 2017-09-07T13:27:36Z

Codecov Report

Merging #791 into develop will decrease coverage by 0.72%.
The diff coverage is 50.72%.

@@ Coverage Diff @@ ## develop #791 +/- ## ============================================ - Coverage 61.13% 60.4% -0.73%  - Complexity 2237 2321 +84  ============================================ Files 449 468 +19 Lines 10801 11464 +663 Branches 640 656 +16 ============================================ + Hits 6603 6925 +322  - Misses 3800 4131 +331  - Partials 398 408 +10

Impacted Files	Coverage Δ	Complexity Δ
...per_cloud/discovery/v1/model/DocumentAccepted.java	`36.36% <ø> (ø)`	`3 <0> (ø)`	⬇️
...eloper_cloud/discovery/v1/model/Configuration.java	`57.89% <ø> (ø)`	`10 <0> (ø)`	⬇️
...d/discovery/v1/model/ListEnvironmentsResponse.java	`50% <ø> (ø)`	`2 <0> (ø)`	⬇️
...eveloper_cloud/discovery/v1/model/FontSetting.java	`5.26% <ø> (ø)`	`1 <0> (ø)`	⬇️
...ud/discovery/v1/model/ListEnvironmentsOptions.java	`69.23% <ø> (ø)`	`2 <0> (ø)`	⬇️
...developer_cloud/discovery/v1/model/Enrichment.java	`68.18% <ø> (ø)`	`8 <0> (ø)`	⬇️
...loper_cloud/discovery/v1/model/DocumentCounts.java	`25% <ø> (ø)`	`1 <0> (ø)`	⬇️
...oud/discovery/v1/model/NormalizationOperation.java	`81.81% <ø> (ø)`	`5 <0> (ø)`	⬇️
...eveloper_cloud/discovery/v1/model/Conversions.java	`30.76% <ø> (ø)`	`3 <0> (ø)`	⬇️
...per_cloud/discovery/v1/model/DocumentSnapshot.java	`25% <ø> (ø)`	`1 <0> (ø)`	⬇️
... and 83 more

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 12b11b8...5589a78. Read the comment docs.

germanattanasio · 2017-09-07T13:45:22Z

I think the last question we had before merging this was if the query request and response were being mapped to object or not.

lpatino10 · 2017-09-07T14:01:41Z

@germanattanasio I'm looking into that now and can add that into this PR when it's done.

lpatino10 · 2017-09-14T17:47:28Z

Here's an example of dealing with aggregations with the new changes:

QueryResponse queryResponse = discovery.query(queryOptions).execute(); Histogram histogram = (Histogram) queryResponse.getAggregations.get(0); Long interval = histogram.getInterval();

This should look the same as it did before we started using generated models. Does this look good @maniax89?

maniax89 · 2017-09-14T18:33:32Z

this looks great! that AggregationSerializer is pretty cool.

commentary - seems like a lot of the response classes have setters on them which i feel like they shouldn't, but maybe not the focus of this PR

lpatino10 · 2017-09-14T18:35:27Z

Thanks! And yeah we'll have to work together to update the models now that we have better support for this. Getting rid of the setters won't be difficult and I also know there are more aggregations to add in the future, for example.

mkistler

Looks very good. I made a few minor comments but nothing that should hold up this PR.

mkistler · 2017-09-15T00:18:46Z

...very/src/main/java/com/ibm/watson/developer_cloud/discovery/v1/model/AddDocumentOptions.java

+ String TEXT_HTML = "text/html";
+ /** application/xhtml+xml. */
+ String APPLICATION_XHTML_XML = "application/xhtml+xml";
+ }


FYI ... the FileMediaType interface will go away with the changes I am making for file handling.

mkistler · 2017-09-15T00:36:46Z

...ery/src/main/java/com/ibm/watson/developer_cloud/discovery/v1/model/QueryNoticesOptions.java

 return this;
 }

+ /**


sort is described in the API Ref as

a comma-separated list of fields in the document to sort on.

So an element of the sort array is a field.

It we want this to be reflected in the SDK, we just need to add an x-item-name annotation on sort with the value "field". Or maybe we would want sort_field. Then the comment above would read

Adds an sort_field to sort

and the method signature below would be

public Builder addSortField(String sortField) {

mkistler · 2017-09-15T00:41:21Z

...ery/src/main/java/com/ibm/watson/developer_cloud/discovery/v1/model/QueryNoticesOptions.java

+ }
+
+ /**
+ * Adds an passagesFields to passagesFields.


This is another place where we might want to use x-item-name, probably just "passagesField".

mkistler · 2017-09-15T00:45:04Z

...ery/src/main/java/com/ibm/watson/developer_cloud/discovery/v1/model/QueryNoticesOptions.java

+ * A query search returns all documents in your data set with full enrichments and full text, but with the most
+ * relevant documents listed first. Use a query search when you want to find the most relevant search results. You
+ * cannot use `natural_language_query` and `query` at the same time.
+ *


If we were writing this SDK by hand, we'd validate that natural_language_query and query are not both specified when constructing the options instance from the builder. There's no support for this in the generator today but we should consider adding it.

mkistler · 2017-09-15T00:52:46Z

.../main/java/com/ibm/watson/developer_cloud/discovery/v1/model/UpdateConfigurationOptions.java

+ this.name = configuration.getName();
+ this.description = configuration.getDescription();
+ this.normalizations = configuration.getNormalizations();
+ this.enrichments = configuration.getEnrichments();


More funky formatting here. This case looks like some spacing issues in the template. But odd that it is showing up here and not in the other classes.

Oh ... I bet this is one of the "alternate-setters". Not in the template ... these lines are produced in the generator itself.

Mike Kistler and others added 8 commits August 13, 2017 08:22

Update generated SDK for Discovery

39647e7

Test changes for updated Discovery SDK

41aa9c8

Update generated SDK to arrange APIs in tag order

6238749

Add unit and integration tests

4dae1d0

The new Training Data endpoint is tested. Some models were also changed to make tests pass, as they were generated wrong. This issue has since been fixed.

Merge pull request #772 from lpatino10/codegen/discovery

14256fe

Add unit and integration tests

Update generated SDK for fix to integer properties in swagger

8385885

Undo change to generated Discovery service file

ce0ab01

Merge pull request #789 from watson-developer-cloud/develop

1b879e2

Update codegen/discovery with the latest changes from develop

germanattanasio requested a review from mkistler September 5, 2017 20:52

Add newest generated code and test for query passages

27c4912

germanattanasio requested a review from maniax89 September 5, 2017 21:43

lpatino10 removed the request for review from maniax89 September 6, 2017 13:48

mkistler approved these changes Sep 6, 2017

View reviewed changes

Merge branch 'develop' into codegen/discovery

1547a58

germanattanasio and others added 2 commits September 7, 2017 15:32

Merge branch 'develop' into codegen/discovery

fd90cc8

Add newest generated code and update tests

5589a78

lpatino10 approved these changes Sep 14, 2017

View reviewed changes

germanattanasio merged commit 2793eaf into develop Sep 14, 2017

mkistler reviewed Sep 15, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Codegen/discovery #791

Codegen/discovery #791

Uh oh!

germanattanasio commented Sep 5, 2017 •

edited by lpatino10

Loading

mkistler left a comment

mkistler Sep 6, 2017

lpatino10 Sep 7, 2017

codecov-io commented Sep 7, 2017 •

edited

Loading

germanattanasio commented Sep 7, 2017

lpatino10 commented Sep 7, 2017

lpatino10 commented Sep 14, 2017

maniax89 commented Sep 14, 2017

lpatino10 commented Sep 14, 2017

mkistler left a comment

mkistler Sep 15, 2017

mkistler Sep 15, 2017

mkistler Sep 15, 2017

mkistler Sep 15, 2017

mkistler Sep 15, 2017

mkistler Sep 15, 2017

Labels

6 participants

Codegen/discovery #791

Codegen/discovery #791

Uh oh!

Conversation

germanattanasio commented Sep 5, 2017 • edited by lpatino10 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

germanattanasio commented Sep 7, 2017

lpatino10 commented Sep 7, 2017

lpatino10 commented Sep 14, 2017

maniax89 commented Sep 14, 2017

lpatino10 commented Sep 14, 2017

mkistler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

6 participants

germanattanasio commented Sep 5, 2017 •

edited by lpatino10

Loading

codecov-io commented Sep 7, 2017 •

edited

Loading