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

First shot on #993 for taxonomies, in particular archives. #2778

Merged
merged 38 commits into from Jun 11, 2017

Conversation

felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 19, 2017

  • I’ve read the guidelines for contributing.
  • I updated AUTHORS.txt and CHANGES.txt (if the change is non-trivial) and documentation (if applicable).
  • I tested my changes.

This PR adds functionality to the taxonomy system to be able to reference translated classifications in taxonomies, with an example how to use this for archives. See #993.

The generated archives currently look ugly due to no CSS :)

@@ -53,6 +53,7 @@ class Archive(Taxonomy):
minimum_post_count_per_classification_in_overview = 1
omit_empty_classifications = False
also_create_classifications_from_other_languages = False
other_language_variable_name = 'other_archive_languages'
Copy link
Member

@Kwpolska Kwpolska May 20, 2017

Choose a reason for hiding this comment

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

Update docs (template-variables.rst).

Copy link
Contributor Author

@felixfontein felixfontein May 20, 2017

Choose a reason for hiding this comment

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

Good point. Thanks for reminding me!

@felixfontein
Copy link
Contributor Author

felixfontein commented May 20, 2017

Ok, basic styles are now there as well.

nikola/utils.py Outdated
# translations themselves
args = {'translation_manager': self, 'site': site,
'posts_per_classification_per_language': posts_per_classification_per_language}
signal('_translations_config'.format(basename.lower())).send(args)
Copy link
Contributor Author

@felixfontein felixfontein May 21, 2017

Choose a reason for hiding this comment

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

Good catch!

@felixfontein
Copy link
Contributor Author

felixfontein commented May 21, 2017

Ok, now there's basic CSS, support for the default themes (base and bootstrap3), and support for sections, tags, categories (with explicit configuration, disabled by default) and authors (enabled by default).

@felixfontein
Copy link
Contributor Author

felixfontein commented May 21, 2017

(Rebasing to master, to get rid of some annoying problems already fixed there.)

Copy link
Member

@Kwpolska Kwpolska left a comment

Pretty good, but I’m not a fan of variable variable names and variable variable existence.

``posts`` list<Post>? List of items for other templates
``kind`` str The classification name
``permalink`` str Permanent link to page
``<other_language_variable_name>`` list<tuple> List of triples ``(other_lang, other_classification, title)``
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

👎 for making even more variable variable names.

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

How about fixing other_languages then?

Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

Much better (see also: #2778 (comment) )

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

Fixed.

# example if they have different meaning:
# [
# {'en': 'cloud', 'de': 'Wolke'}, # the fluffy thing in the sky
# {'en': 'cloud', 'de': 'Cloud'}, # cloud computing
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

How is this going to work? Do English-language readers have to separate sky and butt-computing posts themselves, while the Germans get this done for them?

Perhaps we shouldn’t encourage users to make ambiguous layouts like that, and even prevent duplicate values.

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

That was just the first example which came to my mind :) It could also be the other way around.

The problem is that when you're writing a blog over a longer time, you sooner or later have cases where you use one tag for two different things because the word has multiple meanings. If you start translating stuff, you can end up having that tag split up into two different tags.

While this kind of sucks, I'd really do not want to disallow this. If a user wants to use the same tag for different things (for example, because he has already been doing that for years and only recently noticed), then why not let him do that?

What we can do is not mention this explicitly in a comment, though, if you'd prefer that.

Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Yeah, don’t mention it. The five users who need it will discover it by mistake, or will fix their tags.

Copy link
Contributor Author

@felixfontein felixfontein May 24, 2017

Choose a reason for hiding this comment

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

@Kwpolska: so do you want the examples removed/simplified to not use collisions?

Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Remove lines 360-370, 432-436 and replace them with:

# Format: a list of dicts {language: translation, language2: translation2, …}
# See POSTS_SECTION_TRANSLATIONS example above.

Copy link
Contributor Author

@felixfontein felixfontein May 25, 2017

Choose a reason for hiding this comment

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

Both done.

# If set to True, a tag in a language will be treated as a translation
# of the literally same tag in all other languages. Enable this if you
# do not translate tags, for example.
# TAG_TRANSLATIONS_ADD_DEFAULTS = False
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

I wouldn’t mind a True value for this with uncommenting.

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

I thought that the value in the comment should be the default value, so that if you uncomment it nothing will change (at least in this version). Or is that not the case?

Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

It’s a hack: put one value in nikola.py (oh by the way, you have some work to do in this regard) and another in conf.py.in, so new sites get new behavior, and old sites get something safer.

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

Ok, done.

# For example:
# [
# {'en': 'private', 'de': 'Privat'},
# {'en': 'business', 'fr': 'Arbeit'},
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

last time I checked, die Arbeit was German (and is this the same as business?)

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

LOL, yes, it is. I wanted to look up the French word, but apparently I forgot to do that... Fixed that.

@@ -11,10 +11,24 @@
<link rel="prefetch" href="${posts[0].permalink()}" type="text/html">
% endif
${math.math_styles_ifposts(posts)}
%if other_languages is not UNDEFINED and other_languages:
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

It’d be great if this check was unnecessary, or approached in some different way. (If it’s main index vs taxonomies: pagekind? If it’s taxonomy-dependent: a variable has_other_languages? Define everywhere but make it empty? And of course, you should use True/False, not None/'other_languages')

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

I've added has_other_languages. This means that now list.tmpl requires has_other_languages to be defined, which might be a problem for custom plugins. It looks like none of the standard plugins (both Nikola core and Nikola plugins repositories) do that except taxonomies.

# {'en': 'private', 'de': 'Privat'},
# {'en': 'business', 'fr': 'Arbeit'},
# ]
# SECTION_TRANSLATIONS = []
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

POST_SECTION_TRANSLATIONS

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

Fixed. (I also changed the setting below that accordingly.)

nikola/utils.py Outdated
# Step 3: collapse the tree structure into a linear sorted list,
# with a node coming before its children.

def append_node(classifications, node, path=()):
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

May I request a split of all the hierarchical stuff to a special module? utils.py is quite cluttered, so some special file for those hierarchies will work better. In that case, sort_node, append_node, and perhaps some others, would also be re-usable between functions. (I kinda dislike defining functions inside functions.)

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

This is an inner function because it works on an ad-hoc defined particular data structure which might never be used anywhere again. Making it publicly available (at module level) means that the data structure needs to be documented and written in stone from now on. That's why I used a function in a function.

About splitting the stuff: sure, that could be done, but it might break backwards compatibility if some plugin uses one of these functions. (Not this one, of course, since it's new, but the other, already existing hierarchical stuff.)

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

The split should probably cover the following functions/classes, right?

  • TreeNode
  • clone_treenode
  • flatten_tree_structure
  • sort_classifications
  • parse_escaped_hierarchical_category_name
  • join_hierarchical_category_path

Copy link
Member

@Kwpolska Kwpolska May 24, 2017

Choose a reason for hiding this comment

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

Yes. You can maintain the old API by using a from-import in utils.py.

Copy link
Contributor Author

@felixfontein felixfontein May 24, 2017

Choose a reason for hiding this comment

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

Ok, I'll do that. I think I'll call it hierarchy_utils.py, is that OK for you?

Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Sure.

Copy link
Contributor Author

@felixfontein felixfontein May 24, 2017

Choose a reason for hiding this comment

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

Done.

if site.config.get('{}_TRANSLATIONS_ADD_DEFAULTS'.format(basename), add_defaults_default):
self.add_defaults(posts_per_classification_per_language)
# Use blinker to inform interested parties (plugins) that they can add
# translations themselves
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

Got any ideas for plugins that would do that?

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

If someone wants a more complicated scheme (like using Google Translator to automatically translate tag/category/section/... names), they can use this hook to implement that.

Copy link
Member

@Kwpolska Kwpolska May 24, 2017

Choose a reason for hiding this comment

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

IMO it’s unnecessary.

Copy link
Contributor Author

@felixfontein felixfontein May 24, 2017

Choose a reason for hiding this comment

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

I don't like preventing extensibility if it is so simple and easy to achieve. (And also very efficient. This is only called once per plugin initialization.)

display: none;
}

.translationslist p:before {
Copy link
Member

@Kwpolska Kwpolska May 23, 2017

Choose a reason for hiding this comment

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

You can add that to the existing .metadata p:before.

Copy link
Contributor Author

@felixfontein felixfontein May 23, 2017

Choose a reason for hiding this comment

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

Fixed.

# For example:
# [
# {'en': 'car', 'de': 'Auto'},
# {'en': 'window', 'fr': 'fenêtre'},
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

I’d prefer to see {'en': 'window', 'de': 'Fenster', 'fr': 'fenêtre'} to demonstrate the fact that those dictionaries can contain multiple languages.

Copy link
Contributor Author

@felixfontein felixfontein May 25, 2017

Choose a reason for hiding this comment

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

Since that example is now gone, I added {'en': 'work', 'fr': 'travail', 'de': 'Arbeit'}, in the post sections example.

# example if they have different meaning:
# [
# {'en': 'cloud', 'de': 'Wolke'}, # the fluffy thing in the sky
# {'en': 'cloud', 'de': 'Cloud'}, # cloud computing
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Yeah, don’t mention it. The five users who need it will discover it by mistake, or will fix their tags.

# For example:
# [
# {'en': 'private', 'de': 'Privat'},
# {'en': 'business', 'fr': 'travail'},
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

'en': 'work'

also, I would not duplicate examples — just make one for sections and in other cases, say “refer to post_sections_whatever documentation”.

Copy link
Contributor Author

@felixfontein felixfontein May 25, 2017

Choose a reason for hiding this comment

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

Fixed.

nikola/utils.py Outdated
# Step 3: collapse the tree structure into a linear sorted list,
# with a node coming before its children.

def append_node(classifications, node, path=()):
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Sure.

# example if they have different meaning:
# [
# {'en': 'cloud', 'de': 'Wolke'}, # the fluffy thing in the sky
# {'en': 'cloud', 'de': 'Cloud'}, # cloud computing
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

Remove lines 360-370, 432-436 and replace them with:

# Format: a list of dicts {language: translation, language2: translation2, …}
# See POSTS_SECTION_TRANSLATIONS example above.

@@ -138,6 +140,10 @@ def provide_context_and_uptodate(self, author, lang, node=None):
kw.update(context)
return context, kw

def get_other_language_variants(self, author, lang, classifications_per_language):
Copy link
Member

@Kwpolska Kwpolska May 25, 2017

Choose a reason for hiding this comment

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

(Codacy’s Parameters differ from overridden 'get_other_language_variants' method suggestion here and in other places is pretty sound)

Copy link
Contributor Author

@felixfontein felixfontein May 25, 2017

Choose a reason for hiding this comment

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

Should be fixed now.

Copy link
Contributor Author

@felixfontein felixfontein May 25, 2017

Choose a reason for hiding this comment

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

(Now even a bit more ;) )

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 1, 2017

@Kwpolska: I think everything you mentioned is fixed.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 4, 2017

(I rebased to get rid of several merge conflicts.)

@Kwpolska
Copy link
Member

Kwpolska commented Jun 10, 2017

translation_link_helper is a bad name for this. Make it feed_translation_helper.tmpl with functions renamed eg. feed_head.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

If the helper would only be for feeds, I'd agree. But the whole point of the rename is that it has functions not only for feeds, but also for links to translated HTML pages!

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

(link should be renamed to feed_link, though, since that's the only function dealing only with feeds.)

@Kwpolska
Copy link
Member

Kwpolska commented Jun 10, 2017

feed_ should be included in both the filename and macro names.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

Why should it be in the file name? That makes no sense at all.

@Kwpolska
Copy link
Member

Kwpolska commented Jun 10, 2017

83% of the file is concerned with feeds, not translations.

@getnikola getnikola deleted a comment Jun 10, 2017
@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

I counted. 50 lines are related to translations, 39 lines are related to feeds for blogs with only one language.

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

The template generates links to other representations. These can be feeds, translations, or both.

Signed-off-by: Chris Warrick <kwpolska@gmail.com>
@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

I strongly advise against the new name.

@getnikola getnikola deleted a comment Jun 10, 2017
@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 10, 2017

That should fix the tests.

@getnikola getnikola deleted a comment Jun 10, 2017
@getnikola getnikola deleted a comment Jun 11, 2017
@getnikola getnikola deleted a comment Jun 11, 2017
@Kwpolska Kwpolska added this to the v7.8.8 milestone Jun 11, 2017
@Kwpolska Kwpolska self-assigned this Jun 11, 2017
@getnikola getnikola deleted a comment from felixfontein Jun 11, 2017
@Kwpolska Kwpolska merged commit afe6799 into master Jun 11, 2017
@Kwpolska Kwpolska deleted the taxonomy-lang-refs branch Jun 11, 2017
@Kwpolska
Copy link
Member

Kwpolska commented Jun 11, 2017

Thanks for doing this!

@felixfontein
Copy link
Contributor Author

felixfontein commented Jun 11, 2017

Thanks for reviews and merging :)

@gwax
Copy link
Contributor

gwax commented Jun 12, 2017

@felixfontein thanks for doing this; it's an important thing to get right.

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

Successfully merging this pull request may close these issues.

None yet

3 participants