Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

all: Add TOML support for language files #3257

Merged
merged 4 commits into from Apr 2, 2017
Merged

all: Add TOML support for language files #3257

merged 4 commits into from Apr 2, 2017

Conversation

n10v
Copy link
Contributor

@n10v n10v commented Apr 1, 2017

Fixes #2577
Fixes #3200

@n10v
Copy link
Contributor Author

n10v commented Apr 1, 2017

I will try to figure out tomorrow, why tests are broken.

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

Now it works

@bep
Copy link
Member

bep commented Apr 2, 2017

It looks good, but I suspect you got it to go green by replacing a YAML test variant with the new TOML -- which doesn't really work; we have to support both, even if TOML is better.

This is the error I get when I build my site:

Started building sites ...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x15a8e24]

goroutine 1 [running]:
github.com/spf13/hugo/hugolib.(*Page).shouldBuild(0xc422586a00, 0x17a4470)
	/Users/bep/go/src/github.com/spf13/hugo/hugolib/page.go:781 +0x34
github.com/spf13/hugo/hugolib.(*Site).convertSource(0xc4200deb40, 0x178368c)
	/Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:1237 +0x21a
github.com/spf13/hugo/hugolib.(*Site).createPages(0xc4200deb40, 0x17767e0, 0x9)
	/Users/bep/go/src/github.com/spf13/hugo/hugolib/site.go:1258 +0xc1
github.com/spf13/hugo/hugolib.(*Site).process(0xc4200deb40, 0x101, 0xc420015db8, 0x9a10379202818a0, 0xed072ce2f)

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

It's very strange because it perfectly works with examples/multilingual with yaml files. Something wrong with p.s.Cfg.GetBool() in this case. p.s.Cfg is not nil, but when you call fmt.Println(p.s.Cfg) you can see en and error on this line.

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

Is #3260 somehow related to this error?

@bep
Copy link
Member

bep commented Apr 2, 2017

Doubt it. But I will fix that now and then I can check again.

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

The problem is what we can't see the errors, which go-i18n returns.
To reproduce it you should write invalid translation file and you'll become the same error.

@bep
Copy link
Member

bep commented Apr 2, 2017

I reverted that bad merge, and tested this PR with the new master applied, and still get an error.

Are you saying we are "swallowing" errors from go-i18n somewhere?

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

Yes

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

I figured out, where it is:
site.go:66 : applyDepsIfNeeded returns an error, but we don't use it:

...
  applyDepsIfNeeded(cfg, sites...)

  h.Deps = sites[0].Deps

  return h, nil
}

func applyDepsIfNeeded(cfg deps.DepsCfg, sites ...*Site) error {
...

@bep
Copy link
Member

bep commented Apr 2, 2017

OK, thanks, I will check.

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

I've already fixed it. Everything works. I will push it in 5 minutes

@bep
Copy link
Member

bep commented Apr 2, 2017

But even if I now get better error message, it still is breaking from this PR:

Error: Error building site: Failed to load translations in file "en.yaml": failed to unmarshal en.yaml: yaml: unmarshal errors:
  line 2: cannot unmarshal !!seq into map[string]map[string]interface {}

If I use go-i18n from master, all works fine.

@bep
Copy link
Member

bep commented Apr 2, 2017

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

It's because you have blank line at the beginning of your file. If you delete it, everything will operate normally.
It's a bug of go-i18n, I will make a PR to fix it.

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

nicksnyder/go-i18n#67

@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

Now everything is fixed.

@bep
Copy link
Member

bep commented Apr 2, 2017

Yes, now it works correctly on my site, too.

This is just great, you showed some real persistence landing this. This is a way more important feature than most people will appreciate -- until they start to use it in real projects of some size!

/cc @spf13 @rdwatters @budparr

@bep bep merged commit 27610dd into gohugoio:master Apr 2, 2017
@n10v
Copy link
Contributor Author

n10v commented Apr 2, 2017

Thank you, @bep 😌
But actually it can't be compared to all the work you do. Thank you for supporting this exciting project. I will contribute further to hugo and other side projects to make it better

@n10v n10v deleted the i18n branch April 2, 2017 16:49
@Mawulijo
Copy link

Mawulijo commented Apr 2, 2017

Very glad i am embrassing hugo. I have no idea what was going on but i knew i am in good company.

anthonyfok pushed a commit to anthonyfok/hugo-docs-concept that referenced this pull request Jul 13, 2017
@BoGeM Thanks a lot for your help, sir!
And for the great work on implementing this feature.
I appreciate it! 😄

Fixes rdwatters#64
See PR rdwatters#65 and PR gohugoio/hugo#3257
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Integrate the flatter and simpler lang file structure Add TOML support for language files
3 participants