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

Experiments for generalization of taxonomies. #2535

Merged
merged 132 commits into from Dec 4, 2016
Merged

Conversation

@felixfontein
Copy link
Contributor

felixfontein commented Oct 16, 2016

This is about #2107. I added a new plugin category, Taxonomy, as well as two plugins which take care to classify posts with all Taxonomy plugins and render all associated pages for them.

The next step is re-implementing the following plugins with this framework and see if it works:

  • archive
  • authors
  • indexes (needs to be split up into two plugins, one for the main index and one for section indexes)

It's not possible to recreate the tags plugin with this framework as it contains the option (which is even default!) to put tags and categories onto the same overview page. But that problem will be solved in v8 anyway when categories go away :)

apply_to_posts = True
# Whether this classification applies to pages.
apply_to_pages = False
# The minimum number of posts a classification must have to be listed in

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 16, 2016

Member

Do we have any use case for minimum_post_count_per_classification_in_overview and omit_empty_classifications?

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 16, 2016

Author Contributor

minimum_post_count_per_classification_in_overview will be site.config['TAGLIST_MINIMUM_POSTS'] for tags.

omit_empty_classifications allows to prevent some collisions if you have also_create_classifications_from_other_languages enabled (which seems to be the default for tags and categories in the current implementation).

raise NotImplementedError()

def sort_posts(self, posts, classification, lang):
"""Sort the given list of posts."""

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 16, 2016

Member

Add a note about this and sort_classifications being in-place.

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 16, 2016

Author Contributor

I adjusted that.

For compatibility reasons, the list could be stored somewhere else as well.
In case `has_hierarchy` is `True`, `flat_hierarchy_per_lang` is the flat
hierarchy consisting of `TreeNode` elements, and `hierarchy_lookup_per_lang`

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 16, 2016

Member

utils.TreeNode (which I haven’t known about before)

This comment has been minimized.

Copy link
@felixfontein

felixfontein Oct 16, 2016

Author Contributor

I think I introduced them for category hierarchies.

This comment has been minimized.

Copy link
@Kwpolska

Kwpolska Oct 16, 2016

Member

There are lots of features I never used, and category hierarchies are one of them.

felixfontein added 5 commits Dec 3, 2016
@ralsina
Copy link
Member

ralsina commented Dec 3, 2016

@felixfontein don't worry much about codacy, I was just curious about how it worked. It finds interesting stuff, tho!

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 3, 2016

It did find some unused imports flake8 didn't find. And renaming type to something else might be good, too, as type has a special meaning in Python. So it did make sense to fix these things ;)

@Kwpolska
Copy link
Member

Kwpolska commented Dec 4, 2016

@felixfontein Is this mergeable?

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 4, 2016

Yes, I think so. From my side the branch is complete. The result should get a bit more testing with some more real-life blogs before a new version of Nikola is released, though. I've tested it with my blogs and with yours, but that doesn't cover all edge cases :)

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 4, 2016

@ralsina: now codacy also found some more interesting bugs (see e775b0f). It looks like it's a useful extension to flake8.

@Kwpolska
Copy link
Member

Kwpolska commented Dec 4, 2016

Codacy isn’t some magic. That’s just pylint as a service, which has been configured to be less nitpicky.

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 4, 2016

Should we merge? Or wait some more?

@Kwpolska
Copy link
Member

Kwpolska commented Dec 4, 2016

Please test with nikola-site first. And if you have some spare time, try to increase test coverage of the new features (if you don’t, we’ll merge now).

…disabled taxonomies.
@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 4, 2016

nikola-site now works. I don't really have time now to add more tests; anyway, almost all of the things which might break are related to existing features which were never tested.

@Kwpolska Kwpolska merged commit da0e4dc into master Dec 4, 2016
3 of 5 checks passed
3 of 5 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Kwpolska
Copy link
Member

Kwpolska commented Dec 4, 2016

Merging then. Thanks for all the effort you put in this PR!

@Kwpolska Kwpolska deleted the generalization-of-taxonomies branch Dec 4, 2016
@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 4, 2016

Thanks for all the feedback and merging!

@Kwpolska
Copy link
Member

Kwpolska commented Dec 5, 2016

Here’s an issue, detected with nikola-logs: render_Indexes in DISABLED_PLUGINS won’t work anymore. I had to set INDEX_PATH to something else.

@Kwpolska
Copy link
Member

Kwpolska commented Dec 5, 2016

Uh, that won’t cut it. I had to disable render_taxonomies entirely, or otherwise I’d get a wasteful blog folder (that site uses posts, not pages, and has custom post_list indexes). See #2581.

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 5, 2016

The plugin's name is now classify_indexes, as it doesn't do rendering anymore.

@Kwpolska
Copy link
Member

Kwpolska commented Dec 5, 2016

May I request to restore the old names? That way, we won’t break backwards compatibility with existing blogs.

@felixfontein
Copy link
Contributor Author

felixfontein commented Dec 5, 2016

That doesn't work because there is no 1:1 mapping between old and new names.

I could add some hack which detects the old names in DISABLED_PLUGINS and inserts the corresponding new ones and informs the user.

@Kwpolska
Copy link
Member

Kwpolska commented Dec 5, 2016

👍 for that (let’s move the discussion to #2581).

@felixfontein felixfontein mentioned this pull request Dec 17, 2016
3 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.