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

Finish the multilingual feature #2303

Closed
wants to merge 51 commits into
base: master
from

Conversation

Projects
None yet
@bep
Member

bep commented Jul 26, 2016

This is currently slightly slower than master in a bench mark with 4 fairly big sites:

benchmark           old ns/op      new ns/op      delta
BenchmarkHugo-4     3146033049     3225819889     +2.54%

benchmark           old allocs     new allocs     delta
BenchmarkHugo-4     10557704       10850988       +2.78%

benchmark           old bytes      new bytes      delta
BenchmarkHugo-4     1628022920     1632976488     +0.30%

I guess this is mostly about doing "more work". I have done some general tuning in another PR (see #2378) after some profile work.

  • Add Translations and AllTranslations to Node.
  • Rework the site build process. Looping every site/language and doing the same for all of them kind of works, but isn't very effective nor elegant. Needs to extract the common tasks and do it once only.
  • Rework the documentation.
  • Fix all the TODO(bep) multilingo and TODO(bep) ml comments.
  • Check the relevant URL funcs, absURL etc.
  • Add more tests
  • Write redirect page to default lang on / when multilingual
  • Write a global sitemap.xml
  • Check that taxonomies can be configured on a per language basis
  • Add YAML config test
  • Check sitemap per language
  • Check list command etc.
  • Move shortcode handling to "pre render". The site isn't really ready at the current point where the shortcodes gets rendered, which comes as a surprise to many. This is even more true now with the multilanguage feature (Site.Pages etc. will be empty). So we need to move this handling to a later stage.
  • Watch: Handling adding and removing languages
  • Check Blackfriday config per site
  • Livereload inject
  • Check ref, relref
  • implement #2312
  • Improve the default i18n vs missing bundles and translations
  • See https://github.com/spf13/hugo/pull/2303/files#diff-d7b04948857565053b55f353ccb2bab2R296 while this is a temporary workaround until we get a proper registry for "all nodes and pages", this may be confusing to users expecting the home object in the translations object to be a full blown home. Now even the title is kind of wrong when it is set per language. We should fix this if it isn't too hard. A temporary "node map" maybe.
  • check data in Page.Site.Data from shortcodes + add some data to the ML tests
  • fix variables docs section -- there are some old and not valid info there now
  • Double check all "reuse of nodes for RSS etc"
  • Double check the collection transfers
  • Check i18n template func in shortcode
  • Check shortcode changes vs livereload
  • Fix Go 1.7 vet issues

@bep bep added this to the v0.17 milestone Jul 26, 2016

@bep bep self-assigned this Jul 26, 2016

@the1900

This comment has been minimized.

Show comment
Hide comment
@the1900

the1900 Jul 27, 2016

I feel really pleasurable about this feature. If you don't mind I think taxonomy's should be multilingual too.

When see the taxonomy the url is fixed to domain/taxonomy but if url were domain/lang/taxonomy then it is more flexible I guess.

the1900 commented Jul 27, 2016

I feel really pleasurable about this feature. If you don't mind I think taxonomy's should be multilingual too.

When see the taxonomy the url is fixed to domain/taxonomy but if url were domain/lang/taxonomy then it is more flexible I guess.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Jul 27, 2016

Member

@the1900 I meant it when I said "no comments, please". I have a shitload of work to get the foundation up and running and I don't need "it would be nice if ...".

On a side note, I totally agree about the taxonomies.

Member

bep commented Jul 27, 2016

@the1900 I meant it when I said "no comments, please". I have a shitload of work to get the foundation up and running and I don't need "it would be nice if ...".

On a side note, I totally agree about the taxonomies.

@gohugoio gohugoio locked and limited conversation to collaborators Jul 27, 2016

@gohugoio gohugoio unlocked this conversation Jul 27, 2016

@CLAassistant

This comment has been minimized.

Show comment
Hide comment
@CLAassistant

CLAassistant Aug 5, 2016

CLA assistant check
All committers have signed the CLA.

CLAassistant commented Aug 5, 2016

CLA assistant check
All committers have signed the CLA.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 5, 2016

Member

/cc @hacdias you might want to have a look at this re. your caddy plugin. Shout if you don't understand or if you need something special ...

Member

bep commented Aug 5, 2016

/cc @hacdias you might want to have a look at this re. your caddy plugin. Shout if you don't understand or if you need something special ...

@bep bep changed the title from Work In Progress: Finish the multilingual feature to Finish the multilingual feature Aug 8, 2016

@bep bep added NeedsReview and removed status/in progress labels Aug 8, 2016

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 8, 2016

Member

OK, this should work. A little pragmatic in some areas, but it is well tested.

As to how it works, see

https://github.com/spf13/hugo/blob/d8c372cad52c44f76943f8c5592dc02a1eb65409/docs/content/content/multilingual.md

Happy code review!

/cc @spf13

Member

bep commented Aug 8, 2016

OK, this should work. A little pragmatic in some areas, but it is well tested.

As to how it works, see

https://github.com/spf13/hugo/blob/d8c372cad52c44f76943f8c5592dc02a1eb65409/docs/content/content/multilingual.md

Happy code review!

/cc @spf13

bep added a commit that referenced this pull request Aug 9, 2016

Improve i18n string handling
* Fall back to default language on missing translation file
* Add a i18n-warnings build flag
* If that flag is set, print a parseable and greppable string on missing translation strings

See #2303
@hacdias

This comment has been minimized.

Show comment
Hide comment
@hacdias

hacdias Aug 9, 2016

Contributor

I'll check this out asap. :) Thanks for mentioning.

Contributor

hacdias commented Aug 9, 2016

I'll check this out asap. :) Thanks for mentioning.

@gntech

This comment has been minimized.

Show comment
Hide comment
@gntech

gntech Aug 9, 2016

Contributor

There is some problem reading the Language config when using a yaml-config file.

---
baseurl: "http://replace-this-with-your-hugo-site.com/"
languageCode: "en-us"
title: "My New Hugo Site"

Languages:
  en:
    weight: 1
  fr:
    weight: 2
Error: Error building site: Failed to parse multilingual config: Language config is not a map: map[]

When I try the same config in a toml-format it works as it should.

Contributor

gntech commented Aug 9, 2016

There is some problem reading the Language config when using a yaml-config file.

---
baseurl: "http://replace-this-with-your-hugo-site.com/"
languageCode: "en-us"
title: "My New Hugo Site"

Languages:
  en:
    weight: 1
  fr:
    weight: 2
Error: Error building site: Failed to parse multilingual config: Language config is not a map: map[]

When I try the same config in a toml-format it works as it should.

Example:
```
DefaultContentLanguage = "en"

This comment has been minimized.

@gntech

gntech Aug 9, 2016

Contributor

This is not yaml-syntax

@gntech

gntech Aug 9, 2016

Contributor

This is not yaml-syntax

This comment has been minimized.

@bep

bep Aug 9, 2016

Member

You are right, it is TOML ... Will fix the doc.

@bep

bep Aug 9, 2016

Member

You are right, it is TOML ... Will fix the doc.

This comment has been minimized.

@gntech

gntech Aug 9, 2016

Contributor

Actually I was referring to only line 17. That line is TOML while line 18-32 is YAML

@gntech

gntech Aug 9, 2016

Contributor

Actually I was referring to only line 17. That line is TOML while line 18-32 is YAML

Show outdated Hide outdated docs/content/templates/variables.md Outdated
@gntech

This comment has been minimized.

Show comment
Hide comment
@gntech

gntech Aug 9, 2016

Contributor

I am unable to use my data-files from shortcodes. This was possible before through .Page.Site.Data. Is this a bug or should I use some other variable now with the multilingual feature?

Contributor

gntech commented Aug 9, 2016

I am unable to use my data-files from shortcodes. This was possible before through .Page.Site.Data. Is this a bug or should I use some other variable now with the multilingual feature?

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 9, 2016

Member

@gntech I will check data and add a test for it.

Member

bep commented Aug 9, 2016

@gntech I will check data and add a test for it.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 10, 2016

Member

@gntech see 85345d9 I cannot reproduce it. I would rather say that this was an issue pre this branch. Now we evaluate the shortcodes as late as possible.

Member

bep commented Aug 10, 2016

@gntech see 85345d9 I cannot reproduce it. I would rather say that this was an issue pre this branch. Now we evaluate the shortcodes as late as possible.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 10, 2016

Member

To see what is needed to update a site to multilingual, see this commit from one of my sites:

bep/bep.is@332dbda

It is a one-pager, so maybe not very typical, but you get the drift.

See it in action here:

http://bep.is/

Member

bep commented Aug 10, 2016

To see what is needed to update a site to multilingual, see this commit from one of my sites:

bep/bep.is@332dbda

It is a one-pager, so maybe not very typical, but you get the drift.

See it in action here:

http://bep.is/

@nlsw

This comment has been minimized.

Show comment
Hide comment
@nlsw

nlsw Aug 10, 2016

Config.yaml doesn't work for me: Error: Error building site: Failed to parse multilingual config: Language config is not a map: map[]. I've been using the exact code as in the docs (except the toml issue of @gntech above). Config.toml works great.

nlsw commented Aug 10, 2016

Config.yaml doesn't work for me: Error: Error building site: Failed to parse multilingual config: Language config is not a map: map[]. I've been using the exact code as in the docs (except the toml issue of @gntech above). Config.toml works great.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 10, 2016

Member

@nils-holger ok, I'll add a test for a yaml config, I'll admit I haven't tested that, but thought that should just work.

Member

bep commented Aug 10, 2016

@nils-holger ok, I'll add a test for a yaml config, I'll admit I haven't tested that, but thought that should just work.

Show outdated Hide outdated helpers/url.go Outdated
Show outdated Hide outdated helpers/url.go Outdated
Show outdated Hide outdated hugolib/hugo_sites.go Outdated
func (n *Node) Permalink() string {
return permalink(n.URL())
}

This comment has been minimized.

@moorereason

moorereason Aug 10, 2016

Contributor

I know we do this already on the Page struct (see .Page.Section()), but I don't like these two funcs (URL() & Permalink()). We're embedding URLPath into the Node, which embeds the URL and Permalink fields. Overshadowing those fields with funcs smells bad to me.

We don't need to fix all of Hugo's problems in this PR, but I'd like to see us change the Node struct to have urlPath URLPath and then access n.urlPath.URL. We'd need to add the Slug() and Section() funcs to Node and update code all over the place, but at least we'd avoid this overloading of embedded fields business and some confusion about how those embedded URLPath fields are supposed to be used, esp. with i18n thrown into the mix.

@moorereason

moorereason Aug 10, 2016

Contributor

I know we do this already on the Page struct (see .Page.Section()), but I don't like these two funcs (URL() & Permalink()). We're embedding URLPath into the Node, which embeds the URL and Permalink fields. Overshadowing those fields with funcs smells bad to me.

We don't need to fix all of Hugo's problems in this PR, but I'd like to see us change the Node struct to have urlPath URLPath and then access n.urlPath.URL. We'd need to add the Slug() and Section() funcs to Node and update code all over the place, but at least we'd avoid this overloading of embedded fields business and some confusion about how those embedded URLPath fields are supposed to be used, esp. with i18n thrown into the mix.

This comment has been minimized.

@bep

bep Aug 10, 2016

Member

Yes, I kind of agree ... but ... you will find a lot of stuff that should be done, but I think we should draw some line. I have put a lot of sweat into making better tests, so changes like you suggest should be safer to do. My big motivation for the change above was to get a simple 5 lines template example listing all the translations of a page or a node without all the IsPage and IsHome special cases. I feel that trumps a little code impurity for a while.

@bep

bep Aug 10, 2016

Member

Yes, I kind of agree ... but ... you will find a lot of stuff that should be done, but I think we should draw some line. I have put a lot of sweat into making better tests, so changes like you suggest should be safer to do. My big motivation for the change above was to get a simple 5 lines template example listing all the translations of a page or a node without all the IsPage and IsHome special cases. I feel that trumps a little code impurity for a while.

bep added some commits Aug 10, 2016

Fix YAML loading of multilingual config
And some other minor fixes from code review.

Updates #2309
Create a copy of the section node for RSS
So the Permalink gets correct when listing translations.

I have also checked the other relevant places to make sure we do not overwrite node values we need later.

Pointers can be tricky, but lesson learned is: A copy is cheap.

Updates #2309
Small adjustment to SiteInfo init
After a visual inspection to make (pretty) sure it is correct re multiple languages.

Updates #2309
Set lang template globals for each site when render shortcodes
We should get rid of these globals, but that is another month.
Fix multilingual reload when shortcode changes
This commit also refines the partial rebuild logic, to make sure we do not do more work than needed.

Updates #2309
Add a global Reset func
So we can do some benchmarking.
@gour

This comment has been minimized.

Show comment
Hide comment
@gour

gour Aug 28, 2016

Contributor

I've built 0.17multilingual and after reading the docs tried to run:

hugo --i18n-warnings | grep i18n i18n|MISSING_TRANSLATION|en|wordCount

but it reports about:

Error: unknown flag: --i18n-warnings

What do I miss?

Contributor

gour commented Aug 28, 2016

I've built 0.17multilingual and after reading the docs tried to run:

hugo --i18n-warnings | grep i18n i18n|MISSING_TRANSLATION|en|wordCount

but it reports about:

Error: unknown flag: --i18n-warnings

What do I miss?

@abourget

This comment has been minimized.

Show comment
Hide comment
@abourget

abourget Aug 30, 2016

Contributor

What's lugo?!

Contributor

abourget commented Aug 30, 2016

What's lugo?!

@abourget

This comment has been minimized.

Show comment
Hide comment
@abourget

abourget Aug 30, 2016

Contributor

Would it be useful that I review some more ? Are we good to go with this or does someone else need to take a look ?

I'll be happy to migrate my sites with any changes that were introduced here.. (since m17n) !

Contributor

abourget commented Aug 30, 2016

Would it be useful that I review some more ? Are we good to go with this or does someone else need to take a look ?

I'll be happy to migrate my sites with any changes that were introduced here.. (since m17n) !

@gntech

This comment has been minimized.

Show comment
Hide comment
@gntech

gntech Aug 30, 2016

Contributor

I think we are awaiting @spf to take a look at this and give the official go ahead

Contributor

gntech commented Aug 30, 2016

I think we are awaiting @spf to take a look at this and give the official go ahead

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Aug 31, 2016

Member

@yitzhakbg please use the forum for questions and marketing.

Member

bep commented Aug 31, 2016

@yitzhakbg please use the forum for questions and marketing.

@yitzhakbg

This comment has been minimized.

Show comment
Hide comment
@yitzhakbg

yitzhakbg Aug 31, 2016

Pardon me. Thanks for the correction

Yitzhak Bar Geva https://www.linkedin.com/in/yitzhakbg

On 1 September 2016 at 00:56:27, Bjørn Erik Pedersen (
notifications@github.com) wrote:

@yitzhakbg https://github.com/yitzhakbg please use the forum for
questions and marketing.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2303 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAA49IXiI55Cjn7ShuTZIoR3z7gOoUHMks5qlfh2gaJpZM4JVIOD
.

yitzhakbg commented Aug 31, 2016

Pardon me. Thanks for the correction

Yitzhak Bar Geva https://www.linkedin.com/in/yitzhakbg

On 1 September 2016 at 00:56:27, Bjørn Erik Pedersen (
notifications@github.com) wrote:

@yitzhakbg https://github.com/yitzhakbg please use the forum for
questions and marketing.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#2303 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAA49IXiI55Cjn7ShuTZIoR3z7gOoUHMks5qlfh2gaJpZM4JVIOD
.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 3, 2016

Member

@spf13 has promised to take some time off his schedule to look at this PR. If he does not object to its content in the next few days, I will merge it.

Member

bep commented Sep 3, 2016

@spf13 has promised to take some time off his schedule to look at this PR. If he does not object to its content in the next few days, I will merge it.

@bep bep assigned bep and unassigned spf13 Sep 5, 2016

bep added a commit that referenced this pull request Sep 6, 2016

Improve i18n string handling
* Fall back to default language on missing translation file
* Add a i18n-warnings build flag
* If that flag is set, print a parseable and greppable string on missing translation strings

See #2303
@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Sep 6, 2016

Member

Merged!

Member

bep commented Sep 6, 2016

Merged!

@bep bep closed this Sep 6, 2016

@bep bep deleted the multilingual branch Dec 10, 2016

tychoish added a commit to tychoish/hugo that referenced this pull request Aug 13, 2017

Improve i18n string handling
* Fall back to default language on missing translation file
* Add a i18n-warnings build flag
* If that flag is set, print a parseable and greppable string on missing translation strings

See gohugoio#2303
@shaform

This comment has been minimized.

Show comment
Hide comment
@shaform

shaform Jul 11, 2018

If we have a global sitemap.xml, why don't we have a global robots.txt?

shaform commented Jul 11, 2018

If we have a global sitemap.xml, why don't we have a global robots.txt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment