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

Add section path 2738 #2746

Merged
merged 4 commits into from May 8, 2017
Merged

Add section path 2738 #2746

merged 4 commits into from May 8, 2017

Conversation

@h4ckninja
Copy link
Contributor

@h4ckninja h4ckninja commented May 7, 2017

Pull Request Checklist

  • 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.

Description

Fixes #2738.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 7, 2017

It looks like your commit was misattributed. Please add the e-mail address you used for this commit to your GitHub profile so your commits will appear as yours.

@@ -243,6 +243,12 @@ POSTS_SECTIONS = True
# are the default and will apply GENERATE_ATOM if set.
# POSTS_SECTIONS_ARE_INDEXES = True

# Final locations are:
# output / TRANSLATION[lang] / SECTION_PATH / SECTION_NAME / index.html (list of posts for a section)
# output / TRANSLATION[lang] / SECTION_PATH / rss.xml (RSS feed for a section)
Copy link
Member

@Kwpolska Kwpolska May 7, 2017

That doesn’t make much sense. Did you mean SECTION_PATH / SECTION_NAME / rss.xml? (Otherwise, there would be only one RSS file for all sections)

@@ -98,7 +98,7 @@ def get_classification_friendly_name(self, section, lang, only_last_component=Fa

def get_path(self, section, lang, dest_type='page'):
"""Return a path for the given classification."""
result = [_f for _f in [section] if _f]
result = [_f for _f in [self.site.config['SECTION_PATH'] + section] if _f]
Copy link
Member

@Kwpolska Kwpolska May 7, 2017

Don’t concatenate, make it a list of two elements.

Copy link
Contributor

@felixfontein felixfontein May 7, 2017

You need to write self.site.config['SECTION_PATH'](lang) to make it translatable.

You can take a look at tags.py to see how this could be done correctly:

    def get_path(self, classification, lang, dest_type='page'):
        """Return a path for the given classification."""
        return [_f for _f in [
            self.site.config['TAG_PATH'](lang),
            self.slugify_tag_name(classification, lang)] if _f], 'auto'

So it should be:

        result = [_f for _f in [self.site.config['SECTION_PATH'](lang), section] if _f]

# Final locations are:
# output / TRANSLATION[lang] / SECTION_PATH / SECTION_NAME / index.html (list of posts for a section)
# output / TRANSLATION[lang] / SECTION_PATH / rss.xml (RSS feed for a section)
# (translatable)
Copy link
Contributor

@felixfontein felixfontein May 7, 2017

SECTION_PATH is not translatable in your implementation. You need to add it to nikola/nikola.py, both to the default config and to TRANSLATABLE_SETTINGS. Then, when you use it, you have to provide the language (see comment below). (Search for TAG_PATH in nikola.py to see how it is done.)

@@ -98,7 +98,7 @@ def get_classification_friendly_name(self, section, lang, only_last_component=Fa

def get_path(self, section, lang, dest_type='page'):
"""Return a path for the given classification."""
result = [_f for _f in [section] if _f]
result = [_f for _f in [self.site.config['SECTION_PATH'] + section] if _f]
Copy link
Contributor

@felixfontein felixfontein May 7, 2017

You need to write self.site.config['SECTION_PATH'](lang) to make it translatable.

You can take a look at tags.py to see how this could be done correctly:

    def get_path(self, classification, lang, dest_type='page'):
        """Return a path for the given classification."""
        return [_f for _f in [
            self.site.config['TAG_PATH'](lang),
            self.slugify_tag_name(classification, lang)] if _f], 'auto'

So it should be:

        result = [_f for _f in [self.site.config['SECTION_PATH'](lang), section] if _f]

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 7, 2017

Also, you need to update AUTHORS.txt :)

@h4ckninja
Copy link
Contributor Author

@h4ckninja h4ckninja commented May 7, 2017

It looks like your commit was misattributed. Please add the e-mail address you used for this commit to your GitHub profile so your commits will appear as yours.

Weird, I had that fixed at one point. But should be fixed now. Thanks.

Also, you need to update AUTHORS.txt :)

I didn't because it was such a minor change, but okay.

Did you mean SECTION_PATH / SECTION_NAME / rss.xml?

Yes, that was a typo. I'll fix that.

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 7, 2017

If you'd fix a typo or a small bug, it might be too minor, but this is a new feature after all, even if somewhat minor :)

@h4ckninja
Copy link
Contributor Author

@h4ckninja h4ckninja commented May 7, 2017

Okay, I believe I've addressed the comments.

Copy link
Member

@Kwpolska Kwpolska left a comment

LGTM!

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 8, 2017

LGTM too!

@@ -120,3 +120,4 @@
* `Yaşar Arabacı <https://github.com/yasar11732>`_
* `Zhaojun Meng <https://github.com/zhaojunmeng>`_
* `小明 <https://github.com/dongweiming>`_
* `h4ckninja <https://github.com/h4ckninja>`_
Copy link
Contributor

@felixfontein felixfontein May 8, 2017

Ok, one little thing: you should put yourself at the correct (alphabetically sorted) place ;-) But we (or at least Kwpolska) can do that during merging.

Copy link
Member

@Kwpolska Kwpolska May 8, 2017

Fixed that.

@Kwpolska Kwpolska merged commit 0c77090 into getnikola:master May 8, 2017
3 checks passed
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 8, 2017

Thanks for contributing to Nikola! 🎉

@h4ckninja h4ckninja deleted the add-section-path-2738 branch May 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants