Tone Analyzer: Add support for JSON and HTML input to tone() method

[jeffpk62]

Adds the missing content types, JSON and HTML. Also preserves the functionality of the existing code for backward-compatibility. Adds a number of new examples to demonstrate the new content types. See issue #217.

[jeffpk62]

Not sure what travis/python 2.7 doesn't like about the return statements for the tone() method? I tried to use a ternarary in the request, but that wasn't working, so I took the simple approach of using one of two requests depending on the content type.

[germanattanasio]

@jeffpk62 We are autogenerating this file from the swagger. If the generation isn't working as expected it's probably because the swagger doesn't have the content-type field.

I think @glennrfisher can autogenerate this fix once we fix the swagger. However, I think the examples are good and we should keep them.

[jeffpk62]

@germanattanasio I saw a reference to auto-generation, but the code had also been changed manually. At some point, it appeared that the content-type support was removed, along with a few other changes? So I'm not sure what's happening, but one way or another, we really need to enable this functionality. I'm trying to document the SDK, and it looks pretty bad if the SDK drops features. Perhaps until we determine why the auto-generation isn't doing the right thing, we can include this change? The examples I added won't work without the code changes.

[glennrfisher]

Unfortunately, I'm busy and won't have a chance to review your changes until next week.

For what it's worth, here is a commit where I replaced the generated tone function with the handwritten tone function to avoid breaking changes.

It would be good to add support for multiple content-types, but it might be tricky to do so without a breaking change. For example, on a quick skim of your code, I noticed that the content_type parameter is inserted in the middle of the parameter list. That would be a breaking change, because it would break anyone's existing code that is relying on the parameter order rather than the parameter names.

So before merging any changes, we should make sure that they could not break any existing code. That means it must have the same parameter order, and must send the exact same request when content-type is left unspecified.

[jeffpk62]

@glennrfisher, yep, I should move it to the end. Sorry, I forgot about the optional parameter specification for the positional arguments. All of the existing tests passed, but they all specify the parameter names. I'll submit a fix for that.

Other than that, it should not break existing functionality. One of the reasons there are some odd content-type checks in the code is to ensure that we do the same thing (default to plain text) if the parameter is omitted; otherwise, the code could have been simpler.

I'll submit an update for the order issue, but it's a rather small change. I'd like to have the SDK work the same as the service before releasing a slew of pending updates to the API reference. So if there's any chance you could get to a quick review sooner, that would help me out a lot. Thanks.

[codecov-io]

Codecov Report

Merging #218 into master will decrease coverage by 2.74%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
- Coverage   84.23%   81.49%   -2.75%     
==========================================
  Files          24       24              
  Lines        1161     1167       +6     
==========================================
- Hits          978      951      -27     
- Misses        183      216      +33

Impacted Files	Coverage Δ
watson_developer_cloud/tone_analyzer_v3.py	92.3% <80%> (-7.7%)	⬇️
watson_developer_cloud/language_translator_v2.py	44.82% <0%> (-31.04%)	⬇️
..._developer_cloud/watson_developer_cloud_service.py	63.54% <0%> (-9.38%)	⬇️
watson_developer_cloud/dialog_v1.py	38.88% <0%> (-5.56%)	⬇️
watson_developer_cloud/document_conversion_v1.py	93.75% <0%> (-3.13%)	⬇️

---

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 272412e...66761b9. Read the comment docs.

[jeffpk62]

Fixed placement of content_type parameter and added/updated tests to check for positional correctness.

[germanattanasio]

@jeffpk62 you have a pylint issue.

Module watson_developer_cloud.tone_analyzer_v3
R: 71, 8: Unnecessary "else" after "return" (no-else-return)

Check the line 71 in watson_developer_cloud.tone_analyzer_v3
The build won't pass unless you fix that. I think we can merge this once you fix that.

[jeffpk62]

Hey, @germanattanasio, yeah I saw that on the initial build. The code is set up that way because it can make the request in one of two ways depending on the content type. If it's application/json, we make the request with the first set of parameters; otherwise, we make the request with the second set of parameters. Other than trying some kind of ternary operator on the request, which seemed odd in Python, this seemed like the best way to make the request depending on the content type. I need to do something like this to support the different input options and to maintain backward compatibility. Do you have any ideas on how to rework this? Should I mess with the ternary operator again? Thanks!

[germanattanasio]

Probably better if you write 2 tests even if you will be duplicating code. It's better to have multiple smaller tests

[jeffpk62]

That would be fine if these were in the test cases, @germanattanasio . But this is what I needed to do to accommodate the different content types for the tone() method in the main Tone Analyzer module. I could write a second version of the tone() method, but that just didn't seem like a good idea.

[jeffpk62]

@germanattanasio, I used two if clauses instead of an if/else, and that appears to have gotten past the lint checker. But Python 3.3 is giving me an Assertion error because it reverses the order of the parameters on one of the test calls to the service. All other versions are fine, and py.test passes with my local Python 2.7.13. If I reverse the expected order of the parameters to get past this error with Python 3.3, tests fail for other versions of Python, including my local version. I could remove the assertion from the code, but that seems wrong. Do you have any idea why this might be happening?

[jeffpk62]

@germanattanasio, this is ready for review and hopefully merging. Thanks for the workaround for the nondeterministic parameter assertion.