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

FEATURE: Make fizzle filter operations capable of dealing with arrays #1082

Merged
merged 3 commits into from Sep 7, 2018

Conversation

Projects
6 participants
@p-weisk
Copy link
Contributor

p-weisk commented Sep 20, 2017

This commit amends the implementation for the ^=, $= and *= fizzle operators by checking if the given property is an array and if so, checking whether the filter matches in the following way:

  • *= matches if the array contains an item that matches the filter string exactly.

  • ^= matches if the array's first item matches the filter string exactly.

  • $= matches if the array's last item matches the filter string exactly.

If the given property is not an array the filter operations behave the same way as before.

@daniellienert

This comment has been minimized.

Copy link
Member

daniellienert commented Sep 20, 2017

Hey @p-weisk,
welcome and thanks a lot for your first contribution to the project - its a great pleasure to me personally!
Unfortunately, something went wrong with your PR, looks like you forked your branch from an other branch as you like to merge into. This one is not mergeable.
Also, the change looks more like a feature then a bugfix, as it enables something that was not promised / broken before. I would recommend to target it as a feature on master.

Cheers,
Daniel

@daniellienert daniellienert self-requested a review Sep 20, 2017

@daniellienert
Copy link
Member

daniellienert left a comment

See details in the comment above.

@p-weisk p-weisk changed the base branch from 2.3 to master Sep 21, 2017

@p-weisk p-weisk changed the title BUGFIX: Make fizzle filter operations capable of dealing with arrays FEATURE: Make fizzle filter operations capable of dealing with arrays Sep 21, 2017

@p-weisk

This comment has been minimized.

Copy link
Contributor

p-weisk commented Sep 21, 2017

Hey @daniellienert ,

thanks for your comment and sorry for the broken pull request. I changed the branch to master now. Since I changed it from bugfix to feature now there is no need to merge it into the 2.3 branch anymore, right?

Greetings
Paul

@daniellienert

This comment has been minimized.

Copy link
Member

daniellienert commented Sep 21, 2017

Right.

@p-weisk p-weisk changed the title FEATURE: Make fizzle filter operations capable of dealing with arrays WIP: FEATURE: Make fizzle filter operations capable of dealing with arrays Sep 21, 2017

@p-weisk p-weisk force-pushed the p-weisk:feature/fizzle-filteroperation-array branch from 528a5ca to eda0eea Sep 22, 2017

@p-weisk p-weisk changed the title WIP: FEATURE: Make fizzle filter operations capable of dealing with arrays FEATURE: Make fizzle filter operations capable of dealing with arrays Oct 3, 2017

@albe

This comment has been minimized.

Copy link
Member

albe commented Oct 17, 2017

Thanks for this, looking good from my side. @daniellienert can you revisit your review and possibly approve?

@albe

albe approved these changes Oct 17, 2017

@aertmann

This comment has been minimized.

Copy link
Member

aertmann commented Oct 17, 2017

Hey @p-weisk, thanks for your contribution. I can see the use case for this behavior, although doubt it's a very common one. However the behavior is not intuitive if you ask me and there's nothing similar in jQuery as it DOM doesn't have array properties, hence I'm a bit reluctant about adding it.

Another way to achieve what you're doing is to do s.th. like

*=: ${Array.indexOf(q(node).property('arrayProperty')) !== -1)
^=: ${Array.first(q(node).property('arrayProperty')) === 'abc')
$=: ${Array.last(q(node).property('arrayProperty')) === 'abc')

Would that work for you or does it need to be an EEL filter?

Would like some more opinions on it (cc @skurfuerst @kitsunet). At the very least it needs to be documented.

@dfeyer

This comment has been minimized.

Copy link
Contributor

dfeyer commented Dec 5, 2017

I'm with @aertmann here, mixed feeling ... not sure if we want this in the core ... Array.first/last can be use and the synthax is clean an easy to read.

But there are cases where more advanced operator can make a lots sense, like in postgres your have the @> (contain operator) so you can test is JSON object/array property contains a string like this:

WHERE info->'food' @> '"carrots"';

This kind of operator can make sense. I think we don't need to be too dogmatic about "it does not exist in DOM query" ... because we don't query the DOM here, but we can deal a lots with JSON style structure. So maybe taking some inspiration from Psql can be good, like:

  • ? Does the key/element string exist within the JSON value?
  • ?| Do any of these key/element strings exist?
  • ?& Do all of these key/element strings exist?
@albe

This comment has been minimized.

Copy link
Member

albe commented Dec 6, 2017

I have to say, I like the transport of the known operators "contains" (*=), "starts with" (^=) and "ends with" ($=) to arrays. After all, that's exactly what they do on arrays like they do on strings (which could be considered arrays of characters). The operator is the same there, so it makes sense to use the same syntax and not invent something new (unless we indeed want to extend to something like @dfeyer suggested).

@dfeyer

dfeyer approved these changes Dec 7, 2017

Copy link
Contributor

dfeyer left a comment

Agree with @albe comment, let's make it work string/array the same way

My proposal is for some futur stuff ;)

@dfeyer

This comment has been minimized.

Copy link
Contributor

dfeyer commented Dec 7, 2017

@daniellienert Can you review again ?

@aertmann

This comment has been minimized.

Copy link
Member

aertmann commented Dec 7, 2017

Still not a fan of the proposal I must say. The syntax just doesn't feel natural to me and it can be achieved in other ways as I explained. The point is not that we're not working with the DOM here, but that jQuery struck a good balance between functionality and API surface area. If this is a very common use case that would be difficult to solve in different ways, then the proposal makes sense. I just don't see that being the case here.

Food of thought, this change doesn't include any documentation. I don't think we can really make a decision without seeing how well this proposal fits with the overall documentation/concepts.

Sorry for being the devils advocate here, but we should always be conservative when deciding on a end user API.

@albe

This comment has been minimized.

Copy link
Member

albe commented Dec 7, 2017

Fair enough :) And you are right regarding documentation. We should always expect documentation for new features before accepting.

@albe
Copy link
Member

albe left a comment

Please add documentation.

@albe

albe approved these changes Aug 20, 2018

@albe

This comment has been minimized.

Copy link
Member

albe commented Aug 20, 2018

This has somehow been forgotten. Maybe time to accept this finally in for 5.1? @daniellienert ?

@daniellienert daniellienert merged commit 8c25b83 into neos:master Sep 7, 2018

2 checks passed

continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@albe albe added this to To do in Neos 4.2 & Flow 5.2 Release Board via automation Sep 7, 2018

@albe albe moved this from To do to Done in Neos 4.2 & Flow 5.2 Release Board Sep 7, 2018

@jonnitto

This comment has been minimized.

Copy link
Member

jonnitto commented Dec 6, 2018

This commit also contains unit tests for the array-based filter operations with the following test cases:
1. one array item matches the filter string with the filter operator -> filter should match
2. multiple items match the filter string with the filter operator -> filter should match
3. no item matches the filter string with the filter operator -> filter should not match
4. a substring of an item would match filter string with the filter operator if it wasn't in an array -> filter should not match
5. item would match filter string if different filter operator was used -> filter should not match

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