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

Allowing category hierarchies. #1711

Merged
merged 10 commits into from May 12, 2015
Merged

Allowing category hierarchies. #1711

merged 10 commits into from May 12, 2015

Conversation

@felixfontein
Copy link
Contributor

@felixfontein felixfontein commented May 10, 2015

This is a first shot on implementing category hierarchies, as discussed in #1520. Hierarchies must be explicitly enabled via CATEGORY_ALLOW_HIERARCHIES = True in conf.py. Then / in a category name is used to specify a category path, and \ is used for escaping / and \.

CATEGORY_OUTPUT_FLAT_HIERARCHY can be used to switch between using the full category path to determine the category's slug (when set to False, which is the default), or to just use the subcategory name for the slug (when set to True).

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 10, 2015

(I tried to make the changes to scan_posts as small as possible to make integrating with the refactor-scanposts branch in #1708 as simple as possible.)

@@ -11,6 +11,14 @@
</%block>

<%block name="content">
%if subcategories:
Copy link
Member

@Kwpolska Kwpolska May 10, 2015

wat

Copy link
Contributor Author

@felixfontein felixfontein May 10, 2015

That's because tag_index.tmpl lets index.tmpl do all the work. Usually subcategories is not set, so nothing will happen.
It would be better to move this into tagindex.tmpl, but I was too lazy for that during the first try.

Copy link
Member

@Kwpolska Kwpolska May 10, 2015

add a special block for this to index.tmpl for this and use it in tagindex.tmpl

Copy link
Contributor Author

@felixfontein felixfontein May 10, 2015

Done.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 10, 2015

The invariance check fail in the tests was to be expected, due to the code which creates category hierarchy trees via nested lists.

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 10, 2015

Don’t write Jinja templates by hand. Your if and for declarations contain : at the end. Just use scripts/jinjify.py.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 10, 2015

Fixed that. I forgot where to find that script again...

Apropos, that script won't work with Python 3.

# If CATEGORY_ALLOW_HIERARCHIES is set to True, categories can be organized in
# hierarchies. For a post, the whole path in the hierarchy must be specified,
# using a forward slash ('/') to separate paths. Use a backslash ('\') to escape
# a forward slash or a backslash (i.e. '\//\\' is a path specifying the
Copy link
Member

@ralsina ralsina May 10, 2015

I think we should not enable the insane. It's easier to just use / for separators, and // for a literal /

Copy link
Contributor Author

@felixfontein felixfontein May 10, 2015

But then parsing is not unique, since a///b can be both ['a/', 'b'] and ['a', '/b'].

Copy link
Member

@Kwpolska Kwpolska May 10, 2015

Just parse it left-to-right, like everyone does. a///b['a/', 'b']

Copy link
Member

@Kwpolska Kwpolska May 10, 2015

also, who even wants a / in their category name when they have nesting?

Copy link
Member

@ralsina ralsina May 10, 2015

Good point. I am tempted to say that if someone uses a///b as a tag, he deserves the uncertainty ;-)
I just hate how all the clashing slashes look. Maybe get a 3rd opinion? Otherwise, since you are doing the code, you win.

Copy link
Contributor Author

@felixfontein felixfontein May 10, 2015

Maybe someone likes to have Unix paths in her/his categories, like /dev/null. Having that as a subcategory would be impossible if we parse left-to-right. I really don't like the idea of not being able to specify certain (sub-)category names just because we wanted to make things a little simpler.

Copy link
Member

@ralsina ralsina May 10, 2015

"web/a//b testing" (posts about a/b testing websites)

Copy link
Member

@ralsina ralsina May 10, 2015

I have actually seen /dev/null used as a category name :-P

Ok, you win Felix! (shakes fist)

Copy link
Contributor Author

@felixfontein felixfontein May 10, 2015

Well, most people use neither / nor \ in category names, so essentially nobody will notice this anyway. And the few who really want to use at least one of them, might actually be happy to use what they wanted to use :)

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 10, 2015

@Kwpolska: how to proceed with the (deliberately) failed invariance tests?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 10, 2015

@felixfontein Ignore them. You can’t do anything about it. We’ll trigger a baseline rebuild when we merge this.

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 10, 2015

Out of curiosity: is it something you/we have to do manually, or is it done automatically?

@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented May 10, 2015

@felixfontein semi-automatically: just change the build number in the invariant-builds repo after merging the PR (that’s a race condition and we might need to retry the build for this repo sometimes).

@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 12, 2015

Ok. I'll do some more testing, and if nobody complains until when I'm done, I'll merge it.

@felixfontein felixfontein force-pushed the category_hierarchies branch from 366a0af to ae3c911 May 12, 2015
felixfontein added a commit that referenced this issue May 12, 2015
Allowing category hierarchies. (fixes #1520)
@felixfontein felixfontein merged commit 85617e3 into master May 12, 2015
0 of 3 checks passed
@felixfontein
Copy link
Contributor Author

@felixfontein felixfontein commented May 12, 2015

I cannot commit to the invariant-builds repo, so I cannot trigger a baseline rebuild. @Kwpolska: would you mind doing that? :)

@@ -15,6 +15,8 @@ New in v7.4.1
Features
--------

* Allowing category hierarchies via new option CATEGORY_ALLOW_HIERARCHIES
Copy link
Member

@Kwpolska Kwpolska May 13, 2015

wrong placement, fixed

@felixfontein felixfontein deleted the category_hierarchies branch May 29, 2015
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