Add flatter data file structure #65

n10v · 2017-02-20T21:13:12Z

Fixes #62
Updates #61

TODO:

Add docs

Please leave your opinions and comments.

/cc @bep @moorereason @nicksnyder

nicksnyder

Thanks for the PR! Sorry for the delay in reviewing.

Just a few comments.

nicksnyder · 2017-03-06T05:03:06Z

i18n/bundle/bundle.go

+	var standardFormat []map[string]interface{}
+	err := unmarshal(filename, buf, &standardFormat)
+	if err == nil {
+		translations, err := parseStandardFormat(standardFormat)


this can just be

return parseStandardFormat(standardFormat)

nicksnyder · 2017-03-06T05:03:37Z

i18n/bundle/bundle.go

+	if err := unmarshal(filename, buf, &flatterFormat); err != nil {
+		return nil, err
+	}
+	translations, err := parseFlatterFormat(flatterFormat)


return parseFlatterFormat(flatterFormat)

nicksnyder · 2017-03-06T05:04:59Z

i18n/bundle/bundle.go

 		}
 		translations = append(translations, t)
 	}
 	return translations, nil
 }

+func parseFlatterFormat(data map[string]map[string]interface{}) ([]translation.Translation, error) {


let's just call this parseFlatFormat (and update files to be .flat.json)

nicksnyder · 2017-03-06T05:08:08Z

i18n/bundle/bundle.go

-		if err := unmarshalFunc(buf, &translationsData); err != nil {
-			return nil, fmt.Errorf("failed to load %s because %s", filename, err)
-		}
+		return unmarshalFunc(buf, out)


can you add back the error message? It is nice to know which file the error happened in.

if err := unmarshalFunc(buf, &translationsData); err != nil { return fmt.Errorf("failed to load %s because %s", filename, err) }

nicksnyder · 2017-03-06T05:39:25Z

i18n/translations_test.go

+
+	T, err := b.Tfunc("en-US")
+	if err != nil {
+		panic(err)


no reason to panic in a test, just fail the test

nicksnyder · 2017-03-06T05:40:38Z

i18n/translations_test.go

+	initTestCases()
+}
+
+func initTestCases() {


no need to create a separate method, just do this in init

nicksnyder · 2017-03-06T05:41:57Z

i18n/translations_test.go

+}
+
+func initTestCases() {
+	b := bundle.New()


it doesn't appear necessary to create a bundle during initialization

It is used for creating a Tfunc and Tfunc is used for checking "d_days" in init (person_unread_email_count_timeframe).

Ah, I see. This is weird because you are reading the JSON file during init but testing other formats in the actual tests.

I would actually not try to do anything in init. Since it is just a test, it is fine to re-init the small array of test cases for each test.

nicksnyder · 2017-03-06T05:45:24Z

i18n/translations_test.go

+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.want, func(t *testing.T) {


using Run means tests don't compile on Go < 1.7. Can we do without?

nicksnyder · 2017-03-06T05:46:06Z

goi18n/testdata/en-us.flatter.json

@@ -0,0 +1,34 @@
+{


if this is a copy of en-us.all.json, can we call this en-us.all.flat.json?

nicksnyder · 2017-03-06T05:47:16Z

goi18n/testdata/en-us.flatter.yaml

@@ -0,0 +1,25 @@
+program_greeting:


en-us.all.flat.yaml

nicksnyder · 2017-03-06T16:34:33Z

README.md

@@ -118,6 +118,49 @@ Here is an example of the default file format that go-i18n supports:

 To use a different file format, write a parser for the format and add the parsed translations using [AddTranslation](https://godoc.org/github.com/nicksnyder/go-i18n/i18n#AddTranslation).

+Flat Format


I think we need to update the goi18n merge command to have an option to output the flat format before we advertise this on the main readme. Up to you whether you want to just remove the README update from this PR or update the merge command.

How do you offer to implement this? I propose to add a new boolean flag called -flat to output data in flat format

nicksnyder · 2017-03-06T16:36:30Z

i18n/translations_test.go

+}
+
+func initTestCases() {
+	b := bundle.New()


Ah, I see. This is weird because you are reading the JSON file during init but testing other formats in the actual tests.

I would actually not try to do anything in init. Since it is just a test, it is fine to re-init the small array of test cases for each test.

moorereason · 2017-03-07T02:53:30Z

I'm not familiar with the internals of the JSON and YAML packages. Do they return early if the data structure doesn't match the target interface? Can the caller avoid a double-unmarshal when using the flat format?

n10v · 2017-03-08T19:15:41Z

@moorereason reading the docs of encoding/json and gopkg.in/yaml.v2 I didn't find any method to check the data structure, but in json lib there is a variety of errors (SyntaxError, UnmarshalFieldError, UnmarshalTypeError and etc.), so we can know, if it's flat format or a user just mistook in json syntax (forgot semicolon, colon, curly bracket and etc.)

bep · 2017-03-08T19:28:03Z

This change was motivated by another format, TOML, so having special casings for JSON is sub-optimal.

I think the current implementation is great as is.

If there is a simple way to detect the format ala:

isFlat := bytes.Contains(buf[:100], []byte("someflatidentifier")

Then that would be great, but I would not add it if it adds complexity.

n10v · 2017-03-08T19:47:23Z

The standard format could be detected this way:

isStandard := bytes.Contains(data[:100], []byte("id")) && (bytes.Contains(data[:100], []byte("translation")) || bytes.Contains(data[:100], []byte("translations")))

The problem could be if id will be more than ~95 bytes length, so it's not the right way. But actually this method only reduces complexity.

n10v · 2017-03-09T05:47:43Z

@bep TOML doesn't support standard format layout. That's why we should add flat format
If user passes TOML translation file, it will be automatically parsed as flat format. So we don't need to detect the format for TOML. At least I will implement like that

n10v · 2017-03-09T05:52:08Z

I think, the simplest and right way to detect the format is built on the first byte.
If it's JSON and first byte of data is [ or if it's YAML and first byte is -, then it's standard format, else it's flat format.
What do you think about it?

nicksnyder · 2017-03-09T07:06:47Z

I know TOML isn't part of this PR (and it shouldn't be), but what would the equivalent TOML file look like?

I think, the simplest and right way to detect the format is built on the first byte.
If it's JSON and first byte of data is [ or if it's YAML and first byte is -, then it's standard format, else it's flat format.

This sounds right to me.

n10v · 2017-03-09T07:13:32Z

@nicksnyder

@bep TOML doesn't support standard format layout. That's why we should add flat format
If user passes TOML translation file, it will be automatically parsed as flat format. So we don't need to detect the format for TOML. At least I will implement like that

n10v · 2017-03-15T05:09:24Z

So I updated merge command and made tests for it. What do you think about it?

nicksnyder

@BoGeM Thanks for updating! Just a few more comments and then I will merge it.

Can you also reply with what this will look like in TOML?

nicksnyder · 2017-03-18T21:03:06Z

i18n/bundle/bundle.go

 	case ".json":
-		unmarshalFunc = json.Unmarshal
+		err = json.Unmarshal(buf, out)


just return json.Unmarshal(buf, out)

nicksnyder · 2017-03-18T21:03:11Z

i18n/bundle/bundle.go

 	case ".yaml":
-		unmarshalFunc = yaml.Unmarshal
+		err = yaml.Unmarshal(buf, out)


nicksnyder · 2017-03-18T21:08:46Z

goi18n/merge_command.go

 	sort.Sort(translation.SortableByID(translations))
-	buf, err := marshal(marshalInterface(translations))
+
+	var iTranslations interface{}


not a fan of this variable name. I would just avoid it though

var marshal func ([]translation.Translation) interface{} if mc.flat { marshal = marshalFlatInterface } else { marshal = marshalInterface } buf, err := mc.marshal(marshal(translations))

mc.marshal(marshal(translations)) - it's also very strange.
I offer to name it as conv2I or just convert.

n10v · 2017-03-19T05:08:51Z

@nicksnyder I've already told you: TOML doesn't support standard format layout. That's why we should add flat format. If user passes TOML translation file, it will be automatically parsed as flat format. So we don't need to detect the format for TOML. At least I will implement like that

nicksnyder · 2017-03-19T05:15:27Z

I know TOML doesn't support standard format. I want to know what it will look like in the flat format.

…

On Sat, Mar 18, 2017 at 10:08 PM Albert Nigmatzianov < ***@***.***> wrote: @nicksnyder <https://github.com/nicksnyder> I've already told you: TOML doesn't support standard format layout. That's why we should add flat format. If user passes TOML translation file, it will be automatically parsed as flat format. So we don't need to detect the format for TOML. At least I will implement like that — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuEUMqLtMvN9VlVaeNzwl0laCPU4Tc-ks5rnLhjgaJpZM4MGn4u> .

n10v · 2017-03-19T05:24:06Z

@nicksnyder

func isStandardFormat(ext string, buf []byte) bool {
  if ext == ".toml" {
    return false
  }

  firstRune := rune(buf[0])
  if (ext == ".json" && firstRune == '[') || (ext == ".yaml" && firstRune == '-') {
    return true
  }
  return false
}

nicksnyder · 2017-03-19T19:39:10Z

Sorry if I wasn't clear. I want to know what en-us.flat.toml or perhaps just en-us.toml will look like.

n10v · 2017-03-20T04:04:03Z

Oh sorry. I thought, you asked me about detection of TOML 😅
en-us.flat.toml will look like:

[program_greeting]
other = "Hello world"

[person_greeting]
other = "Hello {{.Person}}"

[my_height_in_meters]
one = "I am {{.Count}} meter tall."
other = "I am {{.Count}} meters tall."

[your_unread_email_count]
one = "You have {{.Count}} unread email."
other = "You have {{.Count}} unread emails."

[person_unread_email_count]
one = "{{.Person}} has {{.Count}} unread email."
other = "{{.Person}} has {{.Count}} unread emails."

[person_unread_email_count_timeframe]
one = "{{.Person}} has {{.Count}} unread email in the past {{.Timeframe}}."
other = "{{.Person}} has {{.Count}} unread emails in the past {{.Timeframe}}."

[d_days]
one = "{{.Count}} day."
other = "{{.Count}} days."

Add flatter format

f693319

nicksnyder requested changes Mar 6, 2017

View reviewed changes

n10v force-pushed the flatter branch 2 times, most recently from 31b1e16 to 72b2d34 Compare March 6, 2017 12:40

nicksnyder reviewed Mar 6, 2017

View reviewed changes

n10v added 3 commits March 8, 2017 16:13

Add more tests

06290a1

Add docs

c1a2bfe

Prevent template parsing if src == ""

dd1acca

Add the logic of flat format to README

620cbe1

Simplify unmarshal in bundle.go

b2c84f0

n10v force-pushed the flatter branch 3 times, most recently from 9720d87 to 37787b5 Compare March 9, 2017 16:16

nicksnyder reviewed Mar 18, 2017

View reviewed changes

n10v added 2 commits March 19, 2017 10:19

Update merge command for flat format

b19bacc

Make format detection more accurate

c569304

n10v force-pushed the flatter branch from 37787b5 to c569304 Compare March 19, 2017 05:21

nicksnyder approved these changes Mar 19, 2017

View reviewed changes

nicksnyder merged commit be79775 into nicksnyder:master Mar 19, 2017

n10v deleted the flatter branch March 20, 2017 04:04

		@@ -118,6 +118,49 @@ Here is an example of the default file format that go-i18n supports:

		To use a different file format, write a parser for the format and add the parsed translations using [AddTranslation](https://godoc.org/github.com/nicksnyder/go-i18n/i18n#AddTranslation).

		Flat Format

Add flatter data file structure #65

Add flatter data file structure #65

Conversation

n10v commented Feb 20, 2017 • edited

nicksnyder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorereason commented Mar 7, 2017

n10v commented Mar 8, 2017 • edited

bep commented Mar 8, 2017 • edited

n10v commented Mar 8, 2017 • edited

n10v commented Mar 9, 2017

n10v commented Mar 9, 2017 • edited

nicksnyder commented Mar 9, 2017

n10v commented Mar 9, 2017 • edited

n10v commented Mar 15, 2017

nicksnyder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n10v commented Mar 19, 2017

nicksnyder commented Mar 19, 2017 via email

n10v commented Mar 19, 2017

nicksnyder commented Mar 19, 2017

n10v commented Mar 20, 2017

n10v commented Feb 20, 2017 •

edited

n10v commented Mar 8, 2017 •

edited

bep commented Mar 8, 2017 •

edited

n10v commented Mar 8, 2017 •

edited

n10v commented Mar 9, 2017 •

edited

n10v commented Mar 9, 2017 •

edited