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

Improving post sorting. #2630

Merged
merged 6 commits into from Jan 11, 2017
Merged

Improving post sorting. #2630

merged 6 commits into from Jan 11, 2017

Conversation

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented Jan 11, 2017

The current sorting of posts for site.timeline, site.posts, site.all_posts, site.pages and the lists of posts per taxonomy has some downsides:

  1. The sorting code is repeated in two different files (nikola.py and taxonomies_classifier.py) and cannot be replaced easily with monkey patching (you have to catch signals and post-process the lists and hope nobody relies on the order not changing since before you began post-processing).
  2. The sorting is done by priority, date, and the post's source path, and all of this reversed; it would make more sense to sort by the post's source path non-reversed, and even better to sort by the post's title (if possible because the language is known, i.e. for taxonomy post lists which are language-dependent) before resorting to the post's source path.

The current PR fixes both things:

  1. It provides a single function in the Nikola site object (which can be easily monkey-patched if someone wants a different sorting order) which sorts posts and returns a new, sorted list.
  2. All the above named places use this function.
  3. It doesn't reverse sort by the post's source path, and it does sort (in correct order) by the title in case priority and date match before resorting to the post's source path in case the language is known (which is the case for taxonomy post lists, as mentioned above).

This should fix the remaining problems in #2627 (i.e. category listings are not random anymore, but in reversed order as one would expect).

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jan 11, 2017

The failing tests are due to the sorting order mattering in the invariance test: until now /pages/bootstrap-demo.html came after /posts/welcome-to-nikola.html (because of reverse sorting by post's source path), but now "Bootstrap Demo" comes before "Welcome to Nikola" (because of correct sorting by post's title).

Copy link
Member

@Kwpolska Kwpolska left a comment

Also needs a changelog entry.

@@ -1984,6 +1984,20 @@ def create_hierarchy(cat_hierarchy, parent=None):
# Next, flatten the hierarchy
self.category_hierarchy = utils.flatten_tree_structure(root_list)

def sort_posts(self, posts, lang=None):
Copy link
Member

@Kwpolska Kwpolska Jan 11, 2017

This is a name clash with nikola.utils.sort_posts. It should be named something distinctive, eg. sort_posts_chronologically (its main purpose)

Moreover, the sort should be done in place (that’s what we did before)

Copy link
Contributor Author

@felixfontein felixfontein Jan 11, 2017

I first wanted to do it inplace, but that's kind of annoying with natsort (you need to use natsort.natsort_keygen, but I'm not sure whether that leads to the same order as natsort.natsorted). But if you insist I can change it back.

Copy link
Member

@Kwpolska Kwpolska Jan 11, 2017

There seems to be no way to end up with the unsorted list, so you can leave it as-is.

Copy link
Contributor Author

@felixfontein felixfontein Jan 11, 2017

Fixed the name clash and added a changelog entry.

@@ -1984,6 +1984,20 @@ def create_hierarchy(cat_hierarchy, parent=None):
# Next, flatten the hierarchy
self.category_hierarchy = utils.flatten_tree_structure(root_list)

def sort_posts(self, posts, lang=None):
Copy link
Contributor Author

@felixfontein felixfontein Jan 11, 2017

It is intentionally not a static method, because it might want to use the site's config in the future.

Copy link
Member

@Kwpolska Kwpolska Jan 11, 2017

What for?

Copy link
Contributor Author

@felixfontein felixfontein Jan 11, 2017

Because maybe someone wants an option to sort things differently. Just a precaution.

Copy link
Member

@Kwpolska Kwpolska Jan 11, 2017

That’s going too far into options-for-five-people territory.

Copy link
Contributor Author

@felixfontein felixfontein Jan 11, 2017

Ok. Fixed.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented Jan 11, 2017

Thanks for improving the documentation!

@Kwpolska Kwpolska merged commit 1438bf0 into master Jan 11, 2017
1 of 5 checks passed
@Kwpolska Kwpolska deleted the improving-post-sorting branch Jan 11, 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

2 participants