add-libretranslate into main (削除) Please don't merge this yet, there are still some things I'd like to discuss and think about a bit more. Feel free to leave a review though. (削除ここまで) This is pretty much ready.
@ -0,0 +31,4 @@
deferresponse.Body.Close()
ifresponse.StatusCode>=400{
Why do >= 400 instead of checking only if it's not 200 ?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Status states that only status codes >=400 convey errors, so it doesn't make sense to assume that anything that's not 200 is an error.
it makes sense to only listen for the reply that you are expecting and that is documented, everything else is unexpected behavior and could very well lead to errors as the code progresses with a faulty reply
Like.. everything between 300 and 399 are redirection messages. It doesn't make sense to progress here like normal, this should result in a different behavior than the standard progression of code.
Okay, I changed the conditions to check for status code 200 only.
@ -0,0 +32,4 @@
deferresponse.Body.Close()
ifresponse.StatusCode>=400{
returnnil,fmt.Errorf("Got status code %d from LibreTranslate API",response.StatusCode)
Error strings should not be capitalized https://github.com/golang/go/wiki/CodeReviewComments#error-strings (same thing for other error strings in this MR)
@ -0,0 +10,4 @@
typeLibreTranslateEnginestruct{
InstanceURLstring
ApiKeystring
should be APIKey https://github.com/golang/go/wiki/CodeReviewComments#initialisms
f5b35a1c57
to 3f1041d6d5
3f1041d6d5
to b19a0aff3f
I added some documentation to LibreTranslateEngine and its fields (and also made a small modification to one of the error messages). Please take a look.
b19a0aff3f
to b031077f23
@Lomanic @metalune please review this PR and leave an approval if this looks good to you, I want to get this merged.
b031077f23
to ed0cdfa15c
Other from those few rather stylistic comments I have, this looks good to me :)
@ -0,0 +23,4 @@
APIKeystring
}
func(_*LibreTranslateEngine)InternalName()string{return"libre"}
Why not make the next two lines variables/static variables? I see no reason for those to be functions
These are required for the TranslationEngine interface, and interfaces can only have methods, not variables/properties/whatever you want to call them. This kind of property-as-method thing is not actually all that uncommon. https://go.dev/doc/effective_go#Getters is relevant. Same thing below.
I see, I feel like there should be a better way, but this is fine for now :)
See my edit, this is actually idiomatic Go style.
@ -0,0 +65,4 @@
func(e*LibreTranslateEngine)TargetLanguages()([]Language,error){returne.getLangs()}
func(_*LibreTranslateEngine)SupportsAutodetect()bool{returntrue}
Why not have this be a variable too?
@ -0,0 +67,4 @@
func(_*LibreTranslateEngine)SupportsAutodetect()bool{returntrue}
typelibreDetectResponse[]struct{
shouldn't all type definitions be at the top of the page? Not sure what the golang style guide says about this, but I feel like it would be more logical to have them at the top of the page
To me, it makes more sense to have these internal types close to where they are used, especially given that each is used in one function only. I'm fine with moving them to the top though.
Alright, I'd say move everything like that to the top, all type defintions and the only thing left at the bottom should be the functions. Other than that this is great.
I went ahead and took a look at the source of a "standard" Go package net/url, and type declarations are interspersed through the code, not at the top. Please take a look, and make of that what you will.
I see, I guess we'll leave it at "define it where you need it, except when you need something more than once across different functions, then define it at the top" because that way one can easily glance at the types that are available in the document
I think it's more nuanced than that (the types in the file I linked to are used by multiple functions but are not at the top), but that's a discussion for another time. :)
bikeshedding for the win
No due date set.
No dependencies set.
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?