Closed Dexus closed 6 years ago
Merging #19 into master will decrease coverage by
7.45%
. The diff coverage is83.38%
.
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 95.91% 88.45% -7.46%
==========================================
Files 3 6 +3
Lines 318 563 +245
==========================================
+ Hits 305 498 +193
- Misses 10 41 +31
- Partials 3 24 +21
Impacted Files | Coverage Δ | |
---|---|---|
translation.go | 100% <100%> (ø) |
|
gotext.go | 100% <100%> (ø) |
:arrow_up: |
helper.go | 100% <100%> (ø) |
|
mo.go | 72.31% <72.31%> (ø) |
|
po.go | 92.98% <90%> (-1.62%) |
:arrow_down: |
locale.go | 94.87% <96.07%> (+0.32%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 8c36835...4172a9c. Read the comment docs.
@Dexus Thanks for all this work! You've been busy!
I've added some reviews and questions to discuss. Maybe we should define the idioms we want to see as external users of the package and work from there.
There are 3 levels of usage for the package:
Po
object)For package functions, I'd like to see something like:
import (
"fmt"
"github.com/leonelquinteros/gotext"
)
func main() {
// Configure package using file-based translators/locales.
// This should work for .mo and .po files automatically.
gotext.Configure("/path/to/locales/root/dir", "en_UK", "domain-name")
// Translate text from default domain
fmt.Println(gotext.Get("My text on 'domain-name' domain"))
// Translate text from a different domain without reconfigure
fmt.Println(gotext.GetD("domain2", "Another text on a different domain"))
}
For Translator objects:
import (
"fmt"
"github.com/leonelquinteros/gotext"
)
func main() {
tr := gotext.NewMoTranslator() // Returns a Translator interface object
tr.ParseFile("/path/to/file.mo")
fmt.Println(tr.Get("Translate this"))
}
And for Locale objects:
import (
"fmt"
"github.com/leonelquinteros/gotext"
)
func main() {
// Create Po based Locale with library path and language code
l := gotext.NewPoLocale("/path/to/locales/root/dir", "es_UY")
// Translate text from default domain
fmt.Println(l.Get("Translate this"))
// Translate text from domain. Domains should be lazy-loaded automatically on first use.
// But include a Locale.LoadDomain(dom string) exported method to allow eager-loading.
fmt.Println(l.GetD("translations", "Translate this"))
}
As you can see, I expect to change as little as possible the package interface. But as we're changing it anyway, and really to the root in some cases, I'm planning all this changes to be the version 2.0.0 of the package.
So undone all the seperation... and most of your points. Are you on a public chat available? So please let me know so we can chat and talk about some points.
It's getting better! I have to review it deeper still, just have been reading the change log. But great job so far, thanks a lot!
Please check test coverage to see if you can increase it before merging.
I'm not in any public chat usually, but we can schedule a talk. I think you're in Germany, correct me if I'm wrong, so please take time zones into account, I'm in Uruguay (UTC-3).
Also please address golint warnings as much as possible:
$ golint ./...
helper.go:16:1: exported function SimplifiedLocale should have comment or be unexported
helper.go:30:1: comment on exported function Printf should be of the form "Printf ..."
mo.go:23:2: exported const MoMagicLittleEndian should have comment (or a comment on this block) or be unexported
mo.go:80:1: exported function NewMoTranslator should have comment or be unexported
mo.go:138:3: struct field MsgIdCount should be MsgIDCount
mo.go:139:3: struct field MsgIdOffset should be MsgIDOffset
mo.go:157:2: var msgIdStart should be msgIDStart
mo.go:158:2: var msgIdLen should be msgIDLen
mo.go:196:3: var msgIdData should be msgIDData
mo.go:336:10: if block ends with a return statement, so drop this else and outdent its block
po.go:81:1: exported function NewPoTranslator should have comment or be unexported
po.go:368:10: if block ends with a return statement, so drop this else and outdent its block
translation.go:8:6: exported type Translation should have comment or be unexported
translation.go:14:1: exported function NewTranslation should have comment or be unexported
translation.go:21:1: exported method Translation.Get should have comment or be unexported
translation.go:33:1: exported method Translation.GetN should have comment or be unexported
translator.go:8:6: exported type Translator should have comment or be unexported
plurals/compiler.go:1:1: package comment should be of the form "Package plurals ..."
plurals/compiler.go:1:1: package comment should not have leading space
plurals/compiler.go:50:2: don't use underscores in Go names; var true_action should be trueAction
plurals/compiler.go:54:2: don't use underscores in Go names; var false_action should be falseAction
plurals/compiler.go:183:9: if block ends with a return statement, so drop this else and outdent its block
plurals/compiler.go:408:9: if block ends with a return statement, so drop this else and outdent its block
plurals/compiler.go:428:9: if block ends with a return statement, so drop this else and outdent its block
plurals/expression.go:38:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:32:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:45:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:58:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:71:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:84:9: if block ends with a return statement, so drop this else and outdent its block
plurals/tests.go:97:9: if block ends with a return statement, so drop this else and outdent its block
@Dexus I've merged this PR as we're far through the main point which is to implement the Mo parser.
I'll create new issues to keep moving towards version 2.0, feel free to address any of these.
Thanks again for all your work! It's really appreciated.
Refactored a bit too, so we can use interfaces to take Mo and Po files
added fixtures
found that the parser for Po files have a bug... but it works... so not touched See: https://www.gnu.org/software/gettext/manual/html_node/PO-Files.html or maybe all "text Po Strings" are "defect"...
What does this change implement?
The MO file is now supported. For this, something had to be refactored so that everything could be implemented cleanly. I think that also makes sense as a basis for future work.
Is this a fix or an improvement?
Have discussed this change in an issue?
Was some test failing because of this issue or change needed?
Are you including tests to cover this change?