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

Complete URL translation #2502

Merged
merged 8 commits into from Oct 14, 2016
Merged

Complete URL translation #2502

merged 8 commits into from Oct 14, 2016

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Sep 19, 2016

Test implementation of my idea in #2116, to allow to override the relative path to the post (relatively to the source folder specified in POSTS/PAGES) using post metadata.

fixes #2116

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Sep 19, 2016

I modified the implementation so that you can use a utils.TranslatableSetting as Post's destination_base. The goal is to make the second argument to PAGES and POSTS a TranslatableSetting. I haven't touched the options yet since I'm not sure if these are used somewhere else as well.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Sep 25, 2016

@ralsina: any thoughts on this?

"""Destination path for this post, relative to output/.

If lang is not specified, it's the current language.
Extension is used in the path if specified.
"""
if lang is None:
lang = nikola.utils.LocaleBorg().current_lang
if _force_source:
folder = self.folder
Copy link
Member

@Kwpolska Kwpolska Sep 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like putting source files in /posts/foo.rst and contents in /whatever/path/says.html. 👎

Copy link
Contributor Author

@felixfontein felixfontein Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. I think I misunderstood something about Post.source_link. Should be fixed now.

@@ -843,7 +859,12 @@ def permalink(self, lang=None, absolute=False, extension='.html', query=None):
extension = self.compiler.extension()

pieces = self.translations[lang].split(os.sep)
pieces += self.folder.split(os.sep)
folder = self.meta[lang].get('path', self.folder_relative)
Copy link
Member

@Kwpolska Kwpolska Sep 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this an instance variable instead of copy-pasting this around and potentially running it multiple times.

Copy link
Contributor Author

@felixfontein felixfontein Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that should be fixed now, too.

@ralsina
Copy link
Member

@ralsina ralsina commented Sep 26, 2016

I am concerned this makes it almost impossible to understand where a post will end up.

So, it would be something like

[translation prefix]/[localized dest folder]/[path metadata]/[slug] ?

elif self.folder_base is not None:
self.folders = {lang: os.path.normpath(os.path.join(self.folder_base(lang), folder)) for lang, folder in self.folders.items()}
self.folder = self.folders[self.default_lang]

Copy link
Member

@ralsina ralsina Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, let me see if I understand this...

  • Post.destination_path will be calculated from the "path" metadata and the destination folder in POSTS which will be translatable.
  • If there is no path metadata, this will be "self.folder_relative". What is that? Does that name make sense?

Copy link
Contributor Author

@felixfontein felixfontein Sep 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's the current behavior.

Whether the name makes sense: I don't know. This is just a test implementation for the idea in #2116.

@ralsina
Copy link
Member

@ralsina ralsina commented Sep 26, 2016

In general, I would prefer if features start as a description of the intended experience, and not as a code dump for everyone else to figure out :-P

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Sep 26, 2016

I’m 👎 for the entire idea. This complicates code and usage. What is the final path? Why is it like this?

And how are users going to find out about this anyway? How many people need this, and where are those translations even involved? This feels like a huge generalisation.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Sep 26, 2016

@ralsina: as mentioned in the PR text, this is a test implementation of the idea described in #2116.

@Kwpolska: there is some interest in having paths be translatable, see #2116. This is just one idea on how to actually implement it. Do you have a better idea?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Sep 27, 2016

I’m afraid I don’t. But I don’t think this really solves the problem it claims it solves. The original issue was “make post paths (sections, mainly) translatable”. This makes (parts of?) the path customizable. Which might solve the problem, but in a round-about way, if the user manually changes the path to the correct section for every post. A real solution would make the section mechanism less dependent on exact output paths (and therefore less fragile).

(I don’t think anyone really cares about translating source URLs.)

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Sep 30, 2016

It doesn't solve the problem completely. If you focus on sections and blog posts, and you actually use sections, then yes, it doesn't really solve it well. If you don't use sections, and don't have pages in a URL hierarchy, you will only need the translatable destination in POSTS and PAGES, but not .. path: xxx.

But if you care about static sites (i.e. pages and not posts), the story is completely different. Think for example about a static page which should have pages like /about/, /about/project/, /about/team/ in another language. In german, you'd like the URLs to be /de/ueber/, /de/ueber/projekt/, /de/ueber/mitarbeiter/ or something like that. That's currently impossible to do with Nikola if you want automatic links to the translated pages. If there's only one layer, like /about/, you can use .. slug: ueber in the german metadata to let it end up in /de/ueber/ instead of /de/about/, but for /about/team/, you can only change the team part with .. slug: mitarbeiter. The resulting URL will be /de/about/mitarbeiter/. The patch in this PR allows you to specify .. path: ueber as well to put the result to /de/ueber/mitarbeiter. Not the best way, but at least it results in good URLs.

(About source URLs: since the code generating source URLs is very similar to the one generating the post/page URL it is simpler to also translate the source URL than not to do that.)

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Sep 30, 2016

So, this really only solves a corner case: when the user wants to change the part between the slug and whatever comes from the language and PAGES settings.

A better solution would be one that’s useful for more things. Especially because the name path is terribly misleading here.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 2, 2016

Well, that's not really true. This also allows to actually solve the problem in a better way for post scanning plugins. The path metadata could also be set by the post scanning plugin after creating the Post object; or it could provide a more specific destination_base for every post which specifies the final path.

Every better solution of this problem MUST be implemented in the post scanning plugin anyway: the post object shouldn't know about such things, and only the post scanning plugin can see more than one post to figure out a potential translated hierarchy.

But without a mechanism to actually allow the post scanning plugin to specify a path based on the language, there's no way to ever solve this.

So moving path out of metadata into something the post scanning plugin provides, or something the post scanning plugin can modify after creating the Post object (for example, by taking it out of the metadata itself) should be there.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

How about just having support for localized dest folders in PAGES and POSTS? Would that be acceptable?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 9, 2016

Any example of how this would look?

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

(That would allow to play around with post scanning plugins which can place the posts depending on locale whereever they want. And it would remove the need for hacks as mentioned in #2116 which try to emulate this.)

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

You mean in the config? Something like:

POSTS = (
    ("posts/*.rst", {"en": "posts", "de": "beitraege"}, "post.tmpl"),
    ("posts/*.md", {"en": "posts", "de": "beitraege"}, "post.tmpl"),
)

The interface to Post's constructor would be similar to the one in this PR, mainly except that you cannot override anything with .. path: xx.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 9, 2016

That sounds better to me. As for the implementation, lines 166-172 in post.py look scary to me.

And perhaps PostScanner plugins should be more concerned with the paths?

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

I'll try to make it less scary, then :)

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

Ok. I hope it's now less scary! I've tested it with a modified demo page, it seems to work.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

(Anyone minds if I do a rebase to get rid of the conflicts and to squash the commits, and then force-push?)

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 9, 2016

Go ahead, just make sure to specify the branch (git push --force origin translated-paths).

Also allowing post scanning plugins to place posts depending on languages.
@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 9, 2016

Now it looks kind of better :)

Kwpolska added 2 commits Oct 14, 2016
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Copy link
Member

@Kwpolska Kwpolska left a comment

Not documented enough. It could also be slightly simpler.

@@ -119,6 +119,9 @@ THEME_COLOR = '#5670d4'
# to feeds, indexes, tag lists and archives and are considered part
# of a blog, while PAGES are just independent HTML pages.
#
# Finally, note that destination can be translated, i.e. you can
# specify a different translation folder per language.
Copy link
Member

@Kwpolska Kwpolska Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs an example here and in manual.

@@ -845,7 +859,8 @@ def permalink(self, lang=None, absolute=False, extension='.html', query=None):
extension = self.compiler.extension()

pieces = self.translations[lang].split(os.sep)
pieces += self.folder.split(os.sep)
folder = self.folders[lang]
pieces += folder.split(os.sep)
Copy link
Member

@Kwpolska Kwpolska Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged into two lines.

Copy link
Contributor Author

@felixfontein felixfontein Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean "merge into one line"?

Copy link
Member

@Kwpolska Kwpolska Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, of course.

Copy link
Contributor Author

@felixfontein felixfontein Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -161,6 +166,14 @@ def __init__(
for lang in sorted(self.translated_to):
default_metadata.update(self.meta[lang])

# Compose paths
Copy link
Member

@Kwpolska Kwpolska Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use an if/else here, perhaps a regular (simpler) for loop instead of convoluted comprehensions with partial items (re-using the first self.folders is just a waste of memory and time)

For example:

if self.folder_base is not None:
    # Use translatable destination folders
    self.folders = {}
    for lang in self.config['TRANSLATIONS'].keys():
        self.folders[lang] = os.path.normpath(os.path.join(
            self.folder_base(lang), self.folder_relative))
else:
    # Old behavior (non-translatable destination path, normalized by scanner)
    self.folders = {lang: self.folder_relative for lang in self.config['TRANSLATIONS'].keys()}

Copy link
Contributor Author

@felixfontein felixfontein Oct 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 14, 2016

👍 from me as long as you add some more documentation.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 14, 2016

Do you think that's enough documentation?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 14, 2016

artikel isn’t a direct translation of pages, is it now?

And perhaps add it to the Multilingual posts section of manual.txt. Merge when you’re done.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 14, 2016

Yes, it's not a direct translation, but what I would use; I'd use articles in english instead of pages as well. Would you prefer a direct translation?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Oct 14, 2016

Do as you please. In English, articles would primarily mean text in a newspaper which corresponds to posts on blogs (and this is what happens on your favorite newspaper’s website: the index page contains articles) — but that’s still a digression.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Oct 14, 2016

I added a somewhat more general paragraph to Multilingual posts.

@felixfontein felixfontein merged commit 3fae233 into master Oct 14, 2016
4 checks passed
@felixfontein felixfontein deleted the translated-paths branch Oct 14, 2016
@Kwpolska Kwpolska added this to the v7.8.2 milestone Oct 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants