Migrate Translator API to v3 #789
Conversation
cleemullins
left a comment
cleemullins
left a comment
There was a problem hiding this comment.
It seems like this PR is doing a few things:
- It's migrating tests from XUnit to MSTest.
- Updating Stylecop warnings across the code base.
- Moving to Integration Tests for some things.
- Migrating from v2 to v3 API
Does the PR need to do all of these things? I would have preferred a single PR for each item to make things easier to track and deal with.
There was a problem hiding this comment.
Same comment on all these files...
There was a problem hiding this comment.
Spelling. SentencesLength
enzocano
force-pushed
the
southworks/translator/migrate-to-v3
branch
from
87dddd0 to
314598c
Compare
|
@cleemullins, we rebased this PR on top of the |
|
This PR looks good to me. I would like to see @amr-elsehemy or @chrimc62 review as well for sanity, as they are the code owners. Once one of them signs off (or someone they suggest) we can merge. We'll then have to integrate the same set of changes into the Node codebase. |
|
Via Mohamed Afify:
|
| { | ||
| public class Alignment | ||
| { | ||
| [JsonProperty("proj")] |
There was a problem hiding this comment.
I suggest changing this to "project" since "proj" is a little bit confusing
There was a problem hiding this comment.
The property reflect the json response from Translator API v3 (link).
| { | ||
| public class DetectedLanguageModel | ||
| { | ||
| [JsonProperty("language")] |
There was a problem hiding this comment.
I suggest adding some documentation for the new classes to become easier to understand.
| @@ -136,71 +110,52 @@ public async Task<List<TranslatedDocument>> TranslateArrayAsync(string[] transla | |||
| } | |||
|
|
|||
| // body of http request | |||
There was a problem hiding this comment.
I don't think this comment is correct now, it was related to the old variable 'body'
amr-elsehemy
left a comment
amr-elsehemy
left a comment
There was a problem hiding this comment.
I looked at the PR, the changes looks fine to me and I'd approve with some suggestions written in my comments inline .
Generally I'd encourage adding some documentation to the new added models so that they would be easier to understand , but I don't think this should block the PR for now .
|
Thanks for your feedback @amr-elsehemy. The added models are not intended to be used outside the translator so we convert them to |
…to-v3 Migrate Translator API to v3
Translator.cscode to use new Translator API v3