Add Microsoft Translator Service support #744

Closed
wants to merge 0 commits into
from

Projects

None yet

6 participants

@marcominerva
Contributor

The new TranslatorServiceClient allows to translate text to various supported languages using the Microsoft Translator Service.

+ /// <seealso cref="Language"/>
+ public TranslatorServiceClient(string subscriptionKey = null)
+ : this(subscriptionKey, CultureInfo.CurrentCulture.Name.ToLower())
+ { }
@deltakosh
deltakosh Jan 3, 2017 Contributor

need line feed

@deltakosh
Contributor

Thanks you so much for this!
Can you just change the image to use the official logo?

@marcominerva
Contributor

Surely! Can you point me to the right icon? Otherwise I'll try to find it :-)

@marcominerva
Contributor

Done :-)

+ /// </summary>
+ public sealed partial class MicrosoftTranslatorPage
+ {
+ private TranslatorServiceClient translatorClient;
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

this should be _translatorClient not translatorClient

+
+ private async void GetLanguages_OnClick(object sender, RoutedEventArgs e)
+ {
+ if (!await Tools.CheckInternetConnectionAsync())
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

You could have used the ConnectionHelper class that the community implemented instead of adding a new one :) could you please used the ConnectionHelper for consistency

@marcominerva
marcominerva Jan 4, 2017 Contributor

The Tools static class isn't mine :-) It was already use by may other Sample Pages. Moreover, internally it uses the ConnectionHelper.

+ dictionary.Add(lang, culture.EnglishName);
+ }
+ }
+ catch
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

why using try catch instead of null checking and dictionary if exist checking !!

@marcominerva
marcominerva Jan 4, 2017 Contributor

Because that are some languages returned by the Translator API, such as zh-CHS, that aren't valid .NET Culture.

+
+ private async void Translate_OnClick(object sender, RoutedEventArgs e)
+ {
+ if (!await Tools.CheckInternetConnectionAsync())
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

same as the other one, please use the connection helper class

@marcominerva
marcominerva Jan 4, 2017 Contributor

Same as above ;-)

+ public AzureAuthToken(string key)
+ {
+ SubscriptionKey = key;
+ _client = new HttpClient();
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

The community created an optimized httpHelper class in the toolkit for making http request, I see it's better to use it

@hermitdave
hermitdave Jan 4, 2017 Collaborator

I agree this should use HttpHelper.Instance rather than creating an instance of HttpClient

+ /// After obtaining a valid token, this class will cache it for this duration.
+ /// Use a duration of 5 minutes, which is less than the actual token lifetime of 10 minutes.
+ /// </summary>
+ private static readonly TimeSpan TokenCacheDuration = new TimeSpan(0, 5, 0);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

why not making the caching configurable and has a default value ? I see it would be great if the developer can control it !!

@marcominerva
marcominerva Jan 4, 2017 Contributor

Because this value is used to control when to refresh authentication token (as reported in the comments). Setting it to an incorrect value can prevent the service to work properly.

+ request.Method = HttpMethod.Post;
+ request.RequestUri = ServiceUrl;
+ request.Content = new StringContent(string.Empty);
+ request.Headers.TryAddWithoutValidation(OcpApimSubscriptionKeyHeader, this.SubscriptionKey);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

this keyword is redundant here, you should remove it.

+ {
+ _client.Dispose();
+ }
+ catch
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

is there is a case for using try catch at the client dispose ?? I see it is redundant also if you used the httpHelper you will not have to worry about managing the httpClient.

+ public AzureAuthToken(string key)
+ {
+ SubscriptionKey = key;
+ _client = new HttpClient();
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

Should we be using our httpclient helper for this?

+ request.Headers.TryAddWithoutValidation(OcpApimSubscriptionKeyHeader, this.SubscriptionKey);
+
+ var response = await _client.SendAsync(request).ConfigureAwait(false);
+ response.EnsureSuccessStatusCode();
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

Given this will throw an exception, do we want to use this?

+ /// </remarks>
+ public class TranslatorServiceClient : ITranslatorServiceClient, IDisposable
+ {
+ private const string _baseUrl = "http://api.microsofttranslator.com/v2/Http.svc/";
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

Constants should not have a _ before them, they should be BaseUrl, or LanguagesUri, etc

+ private const int _maxTextLenght = 1000;
+ private const int _MaxTextLengthForAutoDetection = 100;
+
+ private readonly AzureAuthToken authToken;
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

These should begin with a _

+ public TranslatorServiceClient(string subscriptionKey, string language)
+ {
+ authToken = new AzureAuthToken(subscriptionKey);
+ client = new HttpClient { BaseAddress = new Uri(_baseUrl) };
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

Use our HttpClient helper

+ public async Task<IEnumerable<string>> GetLanguagesAsync()
+ {
+ // Check if it is necessary to obtain/update access token.
+ await this.CheckUpdateTokenAsync().ConfigureAwait(false);
@ScottIsAFool
ScottIsAFool Jan 4, 2017 Collaborator

Remove usages of "this."

@ScottIsAFool
Collaborator

Branch is also out of sync with dev now.

+ /// </remarks>
+ public class TranslatorServiceClient : ITranslatorServiceClient, IDisposable
+ {
+ private const string _baseUrl = "http://api.microsofttranslator.com/v2/Http.svc/";
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

All declared consts at this class are not following the toolkit roles it should be BasedUrl also we are using style cop it should highlighting all this naming warnings instead of memorizing it.
_languagesUri, _translatreUri, etc..

+
+ private readonly AzureAuthToken authToken;
+ private readonly HttpClient client;
+ private string authorizationHeaderValue = string.Empty;
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

Please follow the toolkit guidlines for naming convension
authToken should be _authToken
client should be _client
authrizationHeaderValue should be _authrizationHeaderValue

+ public async Task<IEnumerable<string>> GetLanguagesAsync()
+ {
+ // Check if it is necessary to obtain/update access token.
+ await this.CheckUpdateTokenAsync().ConfigureAwait(false);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

please remove this keyword :)

+
+ var content = await client.GetStringAsync(_languagesUri).ConfigureAwait(false);
+
+ XNamespace ns = "http://schemas.microsoft.com/2003/10/Serialization/Arrays";
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

this could be a const string to be declared at the beginning of class and assigned here.

+ public TranslatorServiceClient(string subscriptionKey, string language)
+ {
+ authToken = new AzureAuthToken(subscriptionKey);
+ client = new HttpClient { BaseAddress = new Uri(_baseUrl) };
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

Wondering if you can use the Toolkit HttpHelper class here :)

+ }
+
+ // Checks if it is necessary to obtain/update access token.
+ await this.CheckUpdateTokenAsync().ConfigureAwait(false);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

please remove this keyword

+ /// </para>
+ /// </remarks>
+ /// <seealso cref="Language"/>
+ public Task<string> TranslateAsync(string text, string to = null) => this.TranslateAsync(text, null, to);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

please remove this keyword

+ text = text.Substring(0, Math.Min(text.Length, _MaxTextLengthForAutoDetection));
+
+ // Checks if it is necessary to obtain/update access token.
+ await this.CheckUpdateTokenAsync().ConfigureAwait(false);
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

please remove this keyword

+ /// </summary>
+ /// <returns>A <see cref="Task"/> that represents the initialize operation.</returns>
+ /// <remarks>Calling this method isn't mandatory, because the token is get/refreshed everytime is needed. However, it is called at startup, it can speed-up subsequest requests.</remarks>
+ public Task InitializeAsync() => this.CheckUpdateTokenAsync();
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

please remove this keyword

+ headers.Add(_AuthorizationUri, authorizationHeaderValue);
+ }
+ }
+ catch
@IbraheemOsama
IbraheemOsama Jan 4, 2017 Collaborator

Why swallowing the exception here, maybe the developer need to know whats wrong so he can fix it or he can report the exception back to the community to find a solution for the problem

@marcominerva
Contributor
marcominerva commented Jan 4, 2017 edited

I have addressed the issues, let me know if I need to change anything else!

@ScottIsAFool
Collaborator

The recent merge you just did seems to have gone awry as you have file changes in this PR now that are unrelated to the PR.

@marcominerva
Contributor

Sorry, I made some mistakes with Git. I will make a new PR with the correct files in minutes :-)

@marcominerva
Contributor

This is the new PR, with only my modified files: #766

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