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

Accept a page argument for taxonomy paths #2589

Merged
merged 10 commits into from Dec 15, 2016
Merged

Conversation

Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Dec 9, 2016

This is #2585, cc @felixfontein. Note the change in the if for pagination — did I just fix a bug or was it intended?

Kwpolska added 3 commits Dec 9, 2016
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
Signed-off-by: Chris Warrick <kwpolska@gmail.com>
page_info = None
if not taxonomy.show_list_as_index and page is not None:
if taxonomy.show_list_as_index and page is not None:
Copy link
Contributor

@felixfontein felixfontein Dec 9, 2016

Choose a reason for hiding this comment

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

Thanks for fixing that bug.

Copy link
Contributor

@felixfontein felixfontein left a comment

LGTM except for Atom path handler.

@@ -269,15 +269,21 @@ def _taxonomy_index_path(self, name, lang, taxonomy):
path, append_index, _ = self._parse_path_result(result)
return self._postprocess_path(path, lang, append_index=append_index, dest_type='list')

def _taxonomy_path(self, name, lang, taxonomy, dest_type='page'):
def _taxonomy_path(self, name, lang, taxonomy, dest_type='page', page=None):
Copy link
Contributor

@felixfontein felixfontein Dec 9, 2016

Choose a reason for hiding this comment

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

You also need a page=None argument for _taxonomy_atom_path (which must be passed to _taxonomy_path).

Copy link
Contributor

@felixfontein felixfontein Dec 9, 2016

Choose a reason for hiding this comment

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

Ah, and in line 247 it should be dest_type in ('page', 'feed') and not dest_type == 'page'.

@felixfontein
Copy link
Contributor

felixfontein commented Dec 9, 2016

There was another problem in _postprocess_path which only showed up when allowing page numbers for Atom feeds, so I decided to simply fix both things.

@felixfontein
Copy link
Contributor

felixfontein commented Dec 9, 2016

The last commit shows how this actually can simplify things. (And it is a test case that the whole thing actually works.)

@Kwpolska Kwpolska requested a review from ralsina Dec 10, 2016
@felixfontein
Copy link
Contributor

felixfontein commented Dec 11, 2016

The last commit (00acea8) isn't completely good. In case the setting INDEXES_PRETTY_PAGE_URL evaluates to True, it doesn't work correctly since it ignores the argument force_addition. That's in fact the only place where force_addition == True, and the argument is only ever needed for page_path and not for page_link.

How about keeping the new code, but only in page_path make a special case which handles force_addition == True? This avoids repeating more logic from the path handlers in the TaxonomiesClassifier plugin, but makes the functions seem more complex.

@Kwpolska
Copy link
Member Author

Kwpolska commented Dec 11, 2016

(“make it work” should be the top priority, not “make it look good”)

@felixfontein
Copy link
Contributor

felixfontein commented Dec 11, 2016

Well, before the commit it worked well, so reverting the commit is a simple solution. On the other hand, I also like to avoid writing logic twice, and using Nikola.link and Nikola.path for the common situation (i.e. when force_addition == False) does that, even though it doesn't look so great. It's kind of a style question.

I guess undoing the commit is the simplest solution which makes it work, though.

… indexes." as that commit doesn't work with INDEXES_PRETTY_PAGE_URL.

This reverts commit 00acea8.
@felixfontein
Copy link
Contributor

felixfontein commented Dec 11, 2016

I decided to undo the commit. (I also fixed an unrelated typo.)

@felixfontein
Copy link
Contributor

felixfontein commented Dec 11, 2016

Oh, another idea: how to add an additional argument to the path handler (like alternative_path), and then use that?

of two integers: the page number and the number of pages. This will
be used to append the correct page number by calling
`utils.adjust_name_for_index_path_list` and
`utils.get_displayed_page_number`.

If `alternative_path` is set to `True`, `utils.adjust_name_for_index_path_list`
Copy link
Member Author

@Kwpolska Kwpolska Dec 12, 2016

Choose a reason for hiding this comment

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

What makes it alternative?

Copy link
Contributor

@felixfontein felixfontein Dec 12, 2016

Choose a reason for hiding this comment

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

It is not the default path (parmalink), but an alternative path. Or it can be; in most cases it's the same.

Do you have a better idea for a name?

Copy link
Member Author

@Kwpolska Kwpolska Dec 12, 2016

Choose a reason for hiding this comment

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

You should explain what the difference would be in the docstring.

Copy link
Contributor

@felixfontein felixfontein Dec 12, 2016

Choose a reason for hiding this comment

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

I added a very short explanation. (I want to avoid repeating the logic of adjust_name_for_index_path_list here.)

@Kwpolska
Copy link
Member Author

Kwpolska commented Dec 15, 2016

@ralsina, any review?

Copy link
Member

@ralsina ralsina left a comment

Quick review LGTM

@Kwpolska Kwpolska merged commit 08f2c0b into master Dec 15, 2016
5 checks passed
@Kwpolska Kwpolska deleted the page-numbers-for-taxonomies branch Dec 15, 2016
@felixfontein
Copy link
Contributor

felixfontein commented Dec 15, 2016

Thanks!

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.

None yet

3 participants