Skip to content

generalize args for NLU#250

Merged
jsstylos merged 2 commits intowatson-developer-cloud:masterfrom
shuisman:master
Sep 28, 2017
Merged

generalize args for NLU#250
jsstylos merged 2 commits intowatson-developer-cloud:masterfrom
shuisman:master

Conversation

@shuisman
Copy link
Copy Markdown
Contributor

At the moment the args are fixed, which is not scalable. In line with other endpoints, this needs to be generalized.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Aug 11, 2017

CLA assistant check
All committers have signed the CLA.

@codecov-io
Copy link
Copy Markdown

codecov-io commented Aug 11, 2017

Codecov Report

Merging #250 into master will decrease coverage by 2.78%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #250      +/-   ##
==========================================
- Coverage   85.23%   82.44%   -2.79%     
==========================================
  Files          24       24              
  Lines        1219     1219              
==========================================
- Hits         1039     1005      -34     
- Misses        180      214      +34
Impacted Files Coverage Δ
...veloper_cloud/natural_language_understanding_v1.py 100% <ø> (ø) ⬆️
watson_developer_cloud/language_translator_v2.py 44.82% <0%> (-31.04%) ⬇️
..._developer_cloud/watson_developer_cloud_service.py 66.66% <0%> (-9.38%) ⬇️
watson_developer_cloud/tone_analyzer_v3.py 92.3% <0%> (-7.7%) ⬇️
watson_developer_cloud/dialog_v1.py 38.88% <0%> (-5.56%) ⬇️
watson_developer_cloud/document_conversion_v1.py 93.75% <0%> (-3.13%) ⬇️
watson_developer_cloud/visual_recognition_v3.py 98.73% <0%> (-1.27%) ⬇️

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 62848a3...0f4840b. Read the comment docs.

@germanattanasio
Copy link
Copy Markdown
Contributor

germanattanasio commented Aug 19, 2017

@shuisman even if you make the arguments dynamic **kwargs they won't do anything because you are not using them in WatsonDeveloperCloudService

@shuisman
Copy link
Copy Markdown
Contributor Author

@germanattanasio, This is true. But at the moment the arguments are fixed. So for each new introduced functionality you have to add them here as well at the moment. With this change makes it future proof and in line with the other endpoints.
In example, x_watson_learning_opt_out is not available at NLU at the moment.

@germanattanasio
Copy link
Copy Markdown
Contributor

@shuisman new functionality doesn't actually change the constructor but If you want to do this you will have to do it for all the service. Right now you are only changing NLU

@jsstylos
Copy link
Copy Markdown
Contributor

We used to only support x_watson_learning_opt_out for Speech, but now I think more services are using it, so it's reasonable to use **kwargs to add it for the services that need it.

@germanattanasio
Copy link
Copy Markdown
Contributor

If we are going to support **kwargs I would prefer to support default headers. Like in the Node SDK

@jsstylos
Copy link
Copy Markdown
Contributor

We should add support for custom headers, and then switch the rest of the services to use **kwargs to support them

@shuisman
Copy link
Copy Markdown
Contributor Author

can this PR be merged please?

@jsstylos jsstylos merged commit c12c9aa into watson-developer-cloud:master Sep 28, 2017
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.

5 participants