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 option require_all_tags to post-list directive. #2665

Merged
merged 3 commits into from Feb 16, 2017

Conversation

@andredias
Copy link
Contributor

@andredias andredias commented Feb 16, 2017

If declared it changes the tag filter behaviour to show only posts that have all specified tags.

If declared it changes the tag filter behaviour to show only posts that have *all* specified tags.
for post in timeline:
post_tags = {t.lower() for t in post.tags}
if compare(tags, post_tags):
filtered_timeline.append(post)
Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

If tags are not specified, filtered_timeline is empty.

Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

technically, if you are making a post list, and require all the tags to be present, and don't offer any tags, you are semantically asking for a list of no posts, so... not incorrect behavior >.>

Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

Uh, no? If no tags are specified, then filtered_timeline stays empty, which means the post list will also be empty. And not every post list is tag-based.

Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

This is already in an if tags: conditional; that case will never actually trigger, or I cant see how it will trigger (now that i see it in more context)

Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

That’s not what I’m talking about. Read the old code: if no tags were specified, filtered_timeline would just be filled with all posts from timeline, otherwise there is a way for continue to fire and not put a post in the timeline. In this code, filtered_timeline may be appended to only if there are tags specified. But if there are no tags specified, the entire block does not fire, and filtered_timeline stays empty. After the tags block, filtered_timeline is used in place of timeline. So, to reiterate: if not tags: filtered_timeline == [] and post list is empty.

Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

Welp, now I feel a little silly. Yeah, this needs an else clause on if tags: ... else: filtered_timeline = timeline[:] # or just = timeline

Copy link
Contributor Author

@andredias andredias Feb 16, 2017

Ok. I'll fix that.

Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

Copying the list does not seem necessary, just setting it to timeline is enough.

Copy link
Member

@Kwpolska Kwpolska left a comment

See line 263. Also needs documentation in the manual and CHANGES.txt.

if tags:
tags = {t.strip().lower() for t in tags.split(',')}
if require_all_tags:
compare = lambda x, y: x.issubset(y)
Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

def compare(x, y) is preferred over assigning lambdas (flake8 test)

Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

Personally, I’d be fine with # NOQA for the first function and compare = operator.and_ for the second.

Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

sounds good to me

Copy link
Member

@Kwpolska Kwpolska Feb 16, 2017

Or wait, the first one can be done with compare = set.issubset.

Copy link
Contributor Author

@andredias andredias Feb 16, 2017

yeah, but it would require defining two additional functions:

if require_all_tags:
    def compare(x, y):
        return x.issubset(y)
else:
    def compare(x, y):
        return x & y

I don't think it is worth it. "Beautiful is better than ugly."

Copy link
Contributor

@tritium21 tritium21 Feb 16, 2017

you could do

    if require_all_tags:
        compare = set.issubset
    else:
        compare = operator.and_

Copy link
Contributor Author

@andredias andredias Feb 16, 2017

Ok. Using operator and set.issubset seems more elegant.

@Kwpolska Kwpolska merged commit a694e45 into getnikola:master Feb 16, 2017
2 of 3 checks passed
@Kwpolska
Copy link
Member

@Kwpolska Kwpolska commented Feb 16, 2017

Thanks for contributing! I simplified the option detection a little in commit 5c2cd6f.

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