Add tests to i18n package

[antham]

No description provided.

[antham]

Just a question, why don't you externalize translations and use yaml or something like that, it would be easy for anyone not knowing go to provide a translation for his/her mother tongue and you could easily provide a ready to fill template ?

[antham]

I know nothing about dutch but is it valid or something forgotten : https://github.com/jesseduffield/lazygit/blob/master/pkg/i18n/dutch.go#L126 => Branch is ?

[jesseduffield]

Thanks for doing this!

see the discussion here around using external files #137. The general idea was that you couldn't bake external files into the binary so it was better to do it in go.

English and Dutch share the word 'is' so that's a valid translation :)

[antham]

Ah ok, so I learnt something about dutch.

Yeah I get it but I was thinking more to a build step something that pick a yaml for instance and dump a structure, maybe something that could take advantage of code generation like here : https://blog.golang.org/generate

[antham]

And also just for your information rakyll did a tool for that purpose (embedding files in a binary) : https://github.com/rakyll/statik the example in the readme is for a web server but I guess you can think of other usages.

[jesseduffield]

I think that setting environment variables here could cause some big problems if tests ever crash mid-way such that the teardown doesn't have the chance to run. Instead, I think we should just define a LanguageDetector interface which implements the 'DetectLanguage()' method and then we can use a wrapper for jibber jabber's method in the app, and define a mock version in the test. I've got a patch that you can apply on this PR to make the necessary code change in the i18n package:

diff --git a/pkg/i18n/i18n.go b/pkg/i18n/i18n.go
index e209d55..611b0bc 100644
--- a/pkg/i18n/i18n.go
+++ b/pkg/i18n/i18n.go
@@ -17,11 +17,20 @@ type Localizer struct {
 	Log           *logrus.Logger
 }

-// NewLocalizer creates a new Localizer
-func NewLocalizer(log *logrus.Logger) (*Localizer, error) {
+type LanguageDetector interface {
+	DetectLanguage() (string, error)
+}
+
+type languageDetector struct{}
+
+func (l *languageDetector) DetectLanguage() (string, error) {
+	return jibber_jabber.DetectLanguage()
+}

+// NewLocalizerWithLanguageDetector creates a localizer using the given langauge detector
+func NewLocalizerWithLanguageDetector(log *logrus.Logger, languageDetector LanguageDetector) (*Localizer, error) {
 	// detect the user's language
-	userLang, err := jibber_jabber.DetectLanguage()
+	userLang, err := languageDetector.DetectLanguage()
 	if err != nil {
 		if err.Error() != "Could not detect Language" {
 			return nil, err
@@ -47,6 +56,11 @@ func NewLocalizer(log *logrus.Logger) (*Localizer, error) {
 	return localizer, nil
 }

+// NewLocalizer creates a new Localizer
+func NewLocalizer(log *logrus.Logger) (*Localizer, error) {
+	return NewLocalizerWithLanguageDetector(log, &languageDetector{})
+}
+
 // Localize handels the translations
 // expects i18n.LocalizeConfig as input: https://godoc.org/github.com/nicksnyder/go-i18n/v2/i18n#Localizer.MustLocalize
 // output: translated string

[antham]

What do you think about moving this :

	// detect the user's language
	userLang, err := languageDetector.DetectLanguage()
	if err != nil {
		if err.Error() != "Could not detect Language" {
			return nil, err
		}
		userLang = "C"
	}
	log.Info("language: " + userLang)

inside the languageDetector

moving that in another function :

	// create a i18n bundle that can be used to add translations and other things
	i18nBundle := &i18n.Bundle{DefaultLanguage: language.English}

	addBundles(log, i18nBundle)

	// return the new localizer that can be used to translate text
	i18nLocalizer := i18n.NewLocalizer(i18nBundle, userLang)

	localizer := &Localizer{
		i18nLocalizer: i18nLocalizer,
		language:      userLang,
		Log:           log,
	}

so something like that (just a draft so don't pay too much attention to details) :

func NewLocalizerWithLanguageDetector(log *logrus.Logger) (*Localizer, error) {
 userLang, err := languageDetector.DetectLanguage()
 
 if err != nil {
   return nil, err  
 }
 
 return setupLocalizer(userLang), nil
}

// Localizer will translate a message into the user's language
type Localizer struct {
	i18nLocalizer *i18n.Localizer
	language      string
	Log           *logrus.Logger
}

type languageDetector struct{}

func (l *languageDetector) DetectLanguage() (string, error) {
	userLang, err := languageDetector.DetectLanguage()
	if err != nil {
		if err.Error() != "Could not detect Language" {
			return nil, err
		}
		userLang = "C"
	}

	return userLang, nil
}


func setupLocalizer(userLang string) *Localizer {
	i18nBundle := &i18n.Bundle{DefaultLanguage: language.English}

	addBundles(log, i18nBundle)

	// return the new localizer that can be used to translate text
	i18nLocalizer := i18n.NewLocalizer(i18nBundle, userLang)

	return &Localizer{
		i18nLocalizer: i18nLocalizer,
		language:      userLang,
		Log:           log,
	}
}

so you could just have a lightweight NewLocalizerWithLanguageDetector, you would be able to instantiate the Localizer for testing purpose and you won't have to ask explicity outside for the LanguageDetector which is an internal dependency

[jesseduffield]

yep that sounds good!

[antham]

@jesseduffield have you seen my comment above about code generation ?

[mjarkk]

@antham The comment you placed looks good to me.
Although it's probably not wrong if someone that is also dutch scans my translations afterwards.
There are probably a view wrong translations because my dutch is shit.
It's mostly technical therms that are probably wrong because there are no dutch words for that.

When translating Branch is off of it will become Branch is afgeleid van if it is Branch is it would be Branch is in dutch is is also is

[ghost]

If you want to mock jibber jabber, you should keep that in the test file, and not in the actual package, otherwise the code becomes tightly coupled to that library. DetectLanguage() and Could not detect Language are both implementation-specific details to that one library.

[antham]

I not sure if I get correctly what you mean, but here in my opinion, you have to isolate that languageDetector.DetectLanguage() which is specific to the library and has to be mockable and the other part which is the error handling specific to the app and which need to be tested.

[ghost]

What I mean is that if for the sake of argument we want to swap out jibber jabber for something else (i.e, dependency injection), languageDetector as an interface like in jesseduffield's example would make more sense. That way whatever driver we write for that library can contain implementation-specific details for whatever "DetectLanguage()" means to that library.

w.r.t. the error, it should either be replaced with COULD_NOT_DETECT_PACKAGE_ERROR_MESSAGE (constant used in the jibber jabber library) or not checked at all. If for whatever reason the language detection library fails, it doesn't matter what the error is, setting the default will still make sense in that case.

[jesseduffield]

Yeah I think we shouldn't check at all. If the DetectLanguage method returns an error, we should log the error and just use 'C'. There's no point handling it in any special way. So we'd end up with:

func NewLocalizerWithLanguageDetector(log *logrus.Logger, languageDetector LanguageDetector)   (*Localizer, error) {
 userLang, err := languageDetector.DetectLanguage()
 if err != nil {
   log.Error(err.Error())
   userLang = 'C'
  }
  
 return setupLocalizer(userLang), nil
}

That way we can pass in the mock in the test where we make DetectLanguage() return whatever we want it to

[jesseduffield]

regarding code generation yeah if we can make that work that'd be awesome. I'm probably not going to have time to look towards implementing myself but if you (or anybody else) is down for it I'll definitely help out however I can

[antham]

@remyabel Yes we can add an interface, but I think it doesn't really serve any purpose. You don't need it to achieve decoupling, you export nothing about that outside and you have really a simple implementation with one-to-one relationships easy to deal with if you change a library for another, I think if things become more complicated (i don't think so but I can get it wrong) you could add interfaces at this time if you really need to.

@jesseduffield I could have a look yes for this generation,

[ghost]

I mentioned interfaces because it's commonly used in mocking libraries, see here.

[antham]

Sure but I can achieve this in another way here. I propose to change the package with what we just talked about to get a clear picture and if you think it's bad, I will change the code.

[antham]

@remyabel @jesseduffield I have rewritten everything following the conversation

[antham]

I added a little refac to addBundles as well

[antham]

As you have the language you can't load only the required translation file ?

[jesseduffield]

@antham you've convinced me! Looks like this is a good pattern: if you only need to mock a function, may as well just let the tested method receive that function. If you need to mock multiple functions, you should wrap stuff in an interface. That sounds reasonable to me.

Good job on the refactor as well!

I'm happy to merge this, but I'll give @remyabel the chance to have a look and give his input

[ghost]

LGTM

[antham]

I would say interface is not a matter of mocking, through interface you achieve decoupling which lead to easier testing but it's a side effect not a primary purpose, the primary purpose of an interface is to be a contract, adding interface is adding an abstraction layer which could lead if you add it without care to have a cumbersome codebase.

[jesseduffield]

@antham that makes sense :)

mergin now