Skip to content

Add model management methods to Natural Language Understanding#209

Merged
jsstylos merged 6 commits intomasterfrom
NLU-models-methods-tests
May 23, 2017
Merged

Add model management methods to Natural Language Understanding#209
jsstylos merged 6 commits intomasterfrom
NLU-models-methods-tests

Conversation

@g-may
Copy link
Copy Markdown
Contributor

@g-may g-may commented May 23, 2017

Successfully listed a custom model from my own service instance and deleted it with these methods.

Copy link
Copy Markdown
Contributor

@kognate kognate left a comment

Choose a reason for hiding this comment

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

one small question.

"""
Deletes a custom model
:param model_id: The ID of the model to delete
:return: dict of analyzed text
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Model deletion really runs a dict of analyzed text?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sure doesn't. thanks

Copy link
Copy Markdown
Contributor

@kognate kognate left a comment

Choose a reason for hiding this comment

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

ok, a couple of more that I should have included in the first pass.

params={"version": self.version},
accept_json=True)

def deleteModel(self, model_id=None):
Copy link
Copy Markdown
Contributor

@kognate kognate May 23, 2017

Choose a reason for hiding this comment

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

You indicate that model_id can be None by making it's default value none. I'd remove the default value and the model_id check later. The service call will error on the None and by making it required the users know it should be there.

Copy link
Copy Markdown
Contributor Author

@g-may g-may May 23, 2017

Choose a reason for hiding this comment

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

I was mirroring how the features parameter is checked by the SDK in the analyze method. Does the new commit fix things?

if model_id is None:
raise ValueError("Missing parameter 'model_id'")

return self.request(method='DELETE', url='/v1/models/'+model_id,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Format strings '/v1/models/{0}'.format(model_id) are preferred over string concatenation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks

@kognate
Copy link
Copy Markdown
Contributor

kognate commented May 23, 2017 via email

@codecov-io
Copy link
Copy Markdown

codecov-io commented May 23, 2017

Codecov Report

Merging #209 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   84.18%   84.23%   +0.05%     
==========================================
  Files          24       24              
  Lines        1157     1161       +4     
==========================================
+ Hits          974      978       +4     
  Misses        183      183
Impacted Files Coverage Δ
...veloper_cloud/natural_language_understanding_v1.py 100% <100%> (ø) ⬆️

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 f81bea7...a3a52ca. Read the comment docs.

@jsstylos jsstylos merged commit 272412e into master May 23, 2017
@germanattanasio germanattanasio deleted the NLU-models-methods-tests branch May 24, 2017 02:46
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.

4 participants