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

fixes list filter #1419

Closed
wants to merge 5 commits into from
Closed

Conversation

tobibeer
Copy link
Contributor

previously did not correctly handle / match the source but only output the list as is

previously did not correctly handle / match the source but only output
the list as is
replace operator.prefix throughout filters?
@Jermolene
Copy link
Member

There are some outstanding filter request you might want to merge first. I mean, using this global function is perhaps not a pressing issue. It will simply improve consistency, while indeed eliminating that inconsistency / bug that existed with the list filter.

Yes, we should wait for those other pull requests to settle down. I'm working my way in reverse chronological order.

@tobibeer
Copy link
Contributor Author

tobibeer commented Feb 3, 2015

I think it would be ok, to take the helper with this pull request and adapt all other filters in a later one... so as to have this issue ticked off.

@@ -30,7 +30,8 @@ function parseFilterOperation(operators,filterString,p) {
operator = {};
// Check for an operator prefix
if(filterString.charAt(p) === "!") {
operator.prefix = filterString.charAt(p++);
operator.prefix = filterString.charAt(p++); // remove?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this line be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for using operator.negate instead

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there's lots of core code using operator.prefix instead, isn't there? I agree that operator.negate is better but we should apply it consistently if we're going to introduce it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we can... one step at a time ...after all, this issue is about the list filter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but separation of concerns would be better if we fixed the list filter independently of introducing operator.negate. it's about having a readable commit history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but this is really about fixing negation and depends on that construct. I would rather not make a pull request with that function first only to use it here.

tobibeer added a commit to tobibeer/TiddlyWiki5 that referenced this pull request Feb 11, 2015
previously did not correctly handle / match the source but only output
the list as is

replaces TiddlyWiki#1419
@tobibeer tobibeer closed this Feb 11, 2015
@tobibeer tobibeer deleted the fix-list-filter branch February 11, 2015 11:02
@tobibeer
Copy link
Contributor Author

closing this, because the refactoring doesn't appear to be desired (in the context of this)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants