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

Taxonomy language link left-overs #2844

Merged
merged 19 commits into from Jun 22, 2017
Merged

Taxonomy language link left-overs #2844

merged 19 commits into from Jun 22, 2017

Conversation

felixfontein
Copy link
Contributor

This completes some things missing in #2778:

  • Adding some missing hreflangs
  • Get rid of also_create_classifications_from_other_languages for tags and categories, and marking this option as deprecated
  • Refactored feed link creation to create correct translated feed links, also simplified code a bit
  • Got rid of rss_link overriding in tags, categories and some more taxonomies, which prevented correct translated RSS feed links
  • Now really all generated .html pages should have links to main RSS and Atom feeds
  • Fixed bug: language was not passed for title and link generation when list.tmpl was used for a classification page

(fixes #993; fixes #2785)

@felixfontein
Copy link
Contributor Author

(The only changes in the baseline are added hreflangs.)

CHANGES.txt Outdated
@@ -5,11 +5,17 @@ Features
--------

* Use ``PRETTY_URLS`` by default on all sites (Issue #1838)
* `also_create_classifications_from_other_languages` is deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Nuke the setting altogether. I doubt there are any third-party taxonomy plugins that depend on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

## Handles both feeds and translations
<%def name="head(classification=None)">
% if rss_link:
<%def name="_append_language(language)">${ " (" + language + ")" if len(translations) > 1 else "" }</%def>
Copy link
Member

Choose a reason for hiding this comment

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

That’s not going to pass jinjification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That reminds me I forgot to run jinjify...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified jinjify in a340ffe so it can handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(FYI: I need this "the whole macro in one line" because it seems to be impossible to get rid of the generated whitespace in Mako otherwise. And if these macros generate superfluous whitespace, this screws up the generated link texts. In Jinja2, this could be done better with whitespace control.)

Copy link
Member

Choose a reason for hiding this comment

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

A bit more code duplication is better than unreadable one-liners.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it would be only a bit more, I'd agree. But here it will be a lot.

Except if I add a function for generating a <link> or <a> which contains all the cases in there and has some more additional variables. I don't know if that's easier to read.

Copy link
Member

Choose a reason for hiding this comment

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

That might look better. _proper screams “this is an ugly hack”

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The _proper was to avoid having a nested if in a one-liner :)

I've now refactored it to avoid one-line macros in 00b2a80 and 1bb186b.

## Handles both feeds and translations
<%def name="head(classification=None, kind='index', feeds=True, other=True, rss_override=True, has_no_feeds=False)">
% if feeds and not has_no_feeds:
${_head_rss(classification, 'index' if (kind == 'archive' and rss_override) else kind, rss_override)}
Copy link
Member

Choose a reason for hiding this comment

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

Jinja probably won’t like this inline if either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That survived jinjification (as opposed to the above) and should work fine in jinja.

## Handles both feeds and translations
<%def name="head(classification=None)">
% if rss_link:
<%def name="_append_language(language)">${ " (" + language + ")" if len(translations) > 1 else "" }</%def>
Copy link
Member

Choose a reason for hiding this comment

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

A bit more code duplication is better than unreadable one-liners.

@@ -16,7 +16,7 @@

<%block name="content">
<%block name="content_header">
${feeds_translations.translation_link()}
${feeds_translations.translation_link(kind)}
Copy link
Member

Choose a reason for hiding this comment

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

Is kind present in /index.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, /index.html is generated by the index taxonomy.

@@ -42,7 +42,7 @@ lang="${lang}">
% if meta_generator_tag:
<meta name="generator" content="Nikola (getnikola.com)">
% endif
${feeds_translations.head()}
${feeds_translations.head(classification=None, kind=kind, other=False)}
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn’t use maybe-defined variables in every file. We should avoid undefined variables if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was actually no need for using kind, since classification is set to None. I've now set kind to 'index', which generates the same result.

@Kwpolska Kwpolska merged commit f9f3c54 into master Jun 22, 2017
@Kwpolska Kwpolska deleted the taxonomy-lang-refs-2 branch June 22, 2017 16:16
@Kwpolska
Copy link
Member

Thanks for doing this!

@felixfontein
Copy link
Contributor Author

Thanks for reviewing and merging! :)

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