sillsdev / machine

Machine is a natural language processing library for .NET that is focused on providing tools for processing resource-poor languages.
MIT License
26 stars 15 forks source link

Serval was down until langtags.json was added back. #194

Closed johnml1135 closed 6 months ago

johnml1135 commented 6 months ago

Serval was down because langtags.json was not available on


This change is Reviewable

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 41 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
The response is disposable, so you should use `using`.

Done.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 42 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You should use the synchronous `Send` method here.

Done.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 46 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You should use the synchronous `CopyTo` method here.

Done.

codecov-commenter commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.22%. Comparing base (448bd1e) to head (96a6d81).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #194 +/- ## ========================================== + Coverage 67.20% 67.22% +0.01% ========================================== Files 441 441 Lines 34924 34941 +17 Branches 4685 4686 +1 ========================================== + Hits 23472 23489 +17 Misses 10362 10362 Partials 1090 1090 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 41 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You shouldn't use `var` here.

Done.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 42 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
This doesn't seem correct. It should be something like this: ``` using HttpResponseMessage response = client.Send( new HttpRequestMessage( HttpMethod.Get, "https://raw.githubusercontent.com/silnrsi/langtags/master/pub/langtags.json" ) ); ```

Done.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 45 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You can make this more concise, i.e. `using FileStream stream = new(cachedAllTagsPath, FileMode.Create);`

Done.

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 53 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You shouldn't call an asynchronous method from a synchronous method. You should use one of the synchronous methods in `HttpContent`.

Done.

johnml1135 commented 6 months ago

tests/SIL.Machine.AspNetCore.Tests/Services/LanguageTagServiceTests.cs line 37 at r4 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
Do we not need to call `Sldr.Initialize()` here as well?

Yes

johnml1135 commented 6 months ago

src/SIL.Machine.AspNetCore/Services/LanguageTagService.cs line 45 at r2 (raw file):

Previously, ddaspit (Damien Daspit) wrote…
You still haven't done this yet. You can update both times you create `FileStream` to just use `new()`.

Done.