add option require_all_tags to post-list directive. #2665
Conversation
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) |
There was a problem hiding this comment.
If tags are not specified, filtered_timeline is empty.
There was a problem hiding this comment.
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 >.>
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Welp, now I feel a little silly. Yeah, this needs an else clause on if tags: ... else: filtered_timeline = timeline[:] # or just = timeline
There was a problem hiding this comment.
Ok. I'll fix that.
There was a problem hiding this comment.
Copying the list does not seem necessary, just setting it to timeline is enough.
| if tags: | ||
| tags = {t.strip().lower() for t in tags.split(',')} | ||
| if require_all_tags: | ||
| compare = lambda x, y: x.issubset(y) |
There was a problem hiding this comment.
def compare(x, y) is preferred over assigning lambdas (flake8 test)
There was a problem hiding this comment.
Personally, I’d be fine with # NOQA for the first function and compare = operator.and_ for the second.
There was a problem hiding this comment.
Or wait, the first one can be done with compare = set.issubset.
There was a problem hiding this comment.
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 & yI don't think it is worth it. "Beautiful is better than ugly."
There was a problem hiding this comment.
you could do
if require_all_tags:
compare = set.issubset
else:
compare = operator.and_
There was a problem hiding this comment.
Ok. Using operator and set.issubset seems more elegant.
|
Thanks for contributing! I simplified the option detection a little in commit 5c2cd6f. |
If declared it changes the tag filter behaviour to show only posts that have all specified tags.