Skip to content

Conversation

@dpopp07
Copy link
Contributor

@dpopp07 dpopp07 commented Sep 11, 2018

Regenerate with latest changes to API definitions and Watson Swagger Codegen.

mediumTaj
mediumTaj previously approved these changes Sep 11, 2018
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!

* @param {string} [params.new_parent] - The ID of the parent dialog node.
* @param {string} [params.new_previous_sibling] - The ID of the previous sibling dialog node.
* @param {Object} [params.new_output] - The output of the dialog node. For more information about how to
* @param {DialogNodeOutput} [params.new_output] - The output of the dialog node. For more information about how to
Copy link
Contributor

Choose a reason for hiding this comment

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

NICE!

@codecov-io
Copy link

codecov-io commented Sep 11, 2018

Codecov Report

Merging #778 into master will decrease coverage by 0.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #778      +/-   ##
==========================================
- Coverage   83.14%   83.13%   -0.02%     
==========================================
  Files          35       35              
  Lines        4498     4513      +15     
  Branches      568      572       +4     
==========================================
+ Hits         3740     3752      +12     
  Misses        363      363              
- Partials      395      398       +3
Impacted Files Coverage Δ
conversation/v1-generated.ts 88.72% <ø> (ø) ⬆️
assistant/v1.ts 89.8% <ø> (ø) ⬆️
language-translator/v3.ts 85.33% <ø> (ø) ⬆️
discovery/v1-generated.ts 72.14% <ø> (ø) ⬆️
language-translator/v2-generated.ts 84.72% <ø> (ø) ⬆️
natural-language-classifier/v1-generated.ts 89.55% <ø> (ø) ⬆️
natural-language-understanding/v1-generated.ts 84.21% <ø> (ø) ⬆️
speech-to-text/v1-generated.ts 84.71% <100%> (+0.19%) ⬆️
visual-recognition/v3-generated.ts 79.5% <62.5%> (-1.2%) ⬇️

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 aa78432...0d1774c. Read the comment docs.

@germanattanasio
Copy link
Contributor

I think you forgot to apply the manual changes for the create classifier in VR. The models are wrong

@dpopp07
Copy link
Contributor Author

dpopp07 commented Sep 12, 2018

@germanattanasio I applied the manual changes, which are just to parse out examples with custom names. What is new is the filename parameters, which the generator now adds to every file type parameter in the SDK.

I can see how this may be problematic, so either I can remove the filename code or edit it so that the code parses custom filenames the same way it's parsing for the examples. Both would require additional manual changes. Let me know what you think

cc @mkistler

* depict the visual subject of any of the classes of the new classifier. Must contain a minimum of 10 images.
*
* Encode special characters in the file name in UTF-8.
* @param {string} [params.classname_positive_examples_filename] - The filename for classname_positive_examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

see this description

* depict the visual subject of any of the classes of the new classifier. Must contain a minimum of 10 images.
*
* Encode special characters in the file name in UTF-8.
* @param {string} [params.classname_positive_examples_filename] - The filename for classname_positive_examples.
Copy link
Contributor

Choose a reason for hiding this comment

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

see this descriptions

@anweshan
Copy link
Contributor

Were the warnings in Visual Recognition generated?

Copy link
Contributor

@anweshan anweshan left a comment

Choose a reason for hiding this comment

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

I am also unsure if VR will work with the difference in manual changes...?

@dpopp07
Copy link
Contributor Author

dpopp07 commented Sep 12, 2018

The warnings are generated when x-include-filename is true in the Swagger, which means the filename is technically required but we didn't want to introduce breaking changes - hence the warning.

The only change in VR is adding the filename params. There are no differences in manual changes. German is suggesting updates to the documentation for the filename params. What do you think won't work?

@dpopp07
Copy link
Contributor Author

dpopp07 commented Sep 12, 2018

@germanattanasio I realized that the filename parameter wouldn't match the positive example parameter name since those are custom. I made some edits to resolve this and would like your opinion. I think this works well but it will make it harder to generate this code in the future. Let me know what you think

@dpopp07 dpopp07 merged commit e662162 into master Sep 13, 2018
@dpopp07 dpopp07 deleted the regenerate-sdk-release-7 branch September 13, 2018 18:10
@watson-github-bot
Copy link
Collaborator

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

7 participants