Skip to content

Conversation

lpatino10
Copy link
Contributor

This PR contains changes to be added for the v6.4.0 release. It uses the following commits:

  • Codegen: 2590ceea86dc97127f9afa67c79a3c321dd68e5c
  • API definition: 053ee5d76d6bdf82c0cb146a98e459dba2e5b0cd

Full details will be in the changelog upon release.

@codecov-io
Copy link

codecov-io commented Aug 13, 2018

Codecov Report

Merging #977 into master will increase coverage by 0.98%.
The diff coverage is 75.46%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #977      +/-   ##
============================================
+ Coverage     39.44%   40.43%   +0.98%     
- Complexity     1609     1707      +98     
============================================
  Files           628      646      +18     
  Lines         15691    15911     +220     
  Branches        877      900      +23     
============================================
+ Hits           6190     6434     +244     
+ Misses         9158     9115      -43     
- Partials        343      362      +19
Impacted Files Coverage Δ Complexity Δ
..._cloud/discovery/v1/model/QueryNoticesOptions.java 35.07% <ø> (ø) 20 <0> (ø) ⬇️
..._language_classifier/v1/model/ClassifyOptions.java 62.5% <ø> (ø) 3 <0> (ø) ⬇️
...loud/discovery/v1/model/FederatedQueryOptions.java 34.02% <ø> (ø) 21 <0> (ø) ⬇️
...eveloper_cloud/speech_to_text/v1/SpeechToText.java 75.21% <ø> (ø) 47 <0> (ø) ⬇️
...veloper_cloud/discovery/v1/model/QueryOptions.java 49.3% <ø> (ø) 22 <0> (ø) ⬇️
...scovery/v1/model/FederatedQueryNoticesOptions.java 34.21% <ø> (ø) 16 <0> (ø) ⬇️
...al_language_classifier/v1/model/ClassifyInput.java 75% <ø> (ø) 2 <0> (ø) ⬇️
...l_language_classifier/v1/model/CollectionItem.java 25% <ø> (ø) 1 <0> (ø) ⬇️
...d/visual_recognition/v3/model/ClassifyOptions.java 45.45% <ø> (ø) 10 <0> (ø) ⬇️
...per_cloud/discovery/v1/model/QueryAggregation.java 0% <ø> (ø) 0 <0> (ø) ⬇️
... and 43 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 a063022...43dd1e3. Read the comment docs.

Copy link
Contributor

@mediumTaj mediumTaj left a comment

Choose a reason for hiding this comment

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

👍 looks good!

private String name;
private String description;
private Long size;
private String size;
Copy link

Choose a reason for hiding this comment

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

I missed the Friday meeting, so I may have heard wrong, but wasn't the decision to leave Size in the SDK as a number for compatibility and add a SizeString parameter to be type String?

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 talking to @lpatino10 yesterday and since the API never returned a value for size we should not be worried about people writing code expecting a value for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You might be right @dpopp07, although then that'll involve us having more handwritten methods for backwards-compatibility in the next major release when the Long size is the only one left.

Copy link

Choose a reason for hiding this comment

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

This is for the request, not the response, right? I know it's been deprecated for a long time, so users shouldn't be including it in their request, but if they are it would probably break them. If you think this is okay and that we shouldn't be causing any issues, I'd prefer to do the same thing in the Node SDK and just change the type. So let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

You can take the same approach as the one logan is taking here.

Copy link

Choose a reason for hiding this comment

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

@lpatino10 I agree, which is why I was I wasn't thrilled about doing it haha
@germanattanasio Sounds good, I'll make that change then

@lpatino10 lpatino10 merged commit 6e5fbf0 into master Aug 14, 2018
@lpatino10 lpatino10 deleted the codegen-updates branch August 14, 2018 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants