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

1159 Add filter expressions for maps and arrays #1163

Merged
merged 2 commits into from Apr 23, 2024

Conversation

michaelhkay
Copy link
Contributor

No description provided.

Copy link
Contributor

@ChristianGruen ChristianGruen left a comment

Choose a reason for hiding this comment

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

A nice proposal…

  • Maybe $value?[predicate] could be changed to value?[predicate], as the first variant implies that the value must be a variable reference.
  • It’s ok that numeric predicates are not supported, but I didn’t understand the reasoning. My assumption would be that $array?[1, 3] and $array?[position() = (1, 3)] are simply equivalent.
  • Typo: specificd

@michaelhkay
Copy link
Contributor Author

Fix #1159

@michaelhkay michaelhkay added Feature A change that introduces a new feature Tests Needed Tests need to be written or merged labels Apr 19, 2024
@michaelhkay
Copy link
Contributor Author

I dropped numeric predicates because I felt people would expect ["a","b","c"]?[1] to return "a" rather than ["a"], but then what would ["a","b","c"]?[1,2] return? And there are plenty of other ways of selecting a single member of an array, we don't need another.

@ChristianGruen
Copy link
Contributor

I dropped numeric predicates because I felt people would expect ["a","b","c"]?[1] to return "a" rather than ["a"], but then what would ["a","b","c"]?[1,2] return?

Would an implementation then be allowed to return the full array for ["a","b","c"]?[1, 2], i.e., would it be one of those cases in which an error happens, but which doesn’t necessarily have to be reported (see also #1014 (comment))?

Ironically, people might still write $array?[1] and be surprised that the input array is returned unchanged. If we want to reduce EBV confusion (see the discussion in #829 (comment) and others), we could require the map/array filter predicate to return a boolean value:

(: valid :)
$array?[position() = 1]
$array?[true()]
[true()]?[?1]
['a']?[boolean(?1)]

(: invalid :)
$array?[1]
$array?['a']
$array?[./a]
['a']?[?1]

@michaelhkay
Copy link
Contributor Author

Minor detail: should "?[" be one token or two? I can't see any strong arguments either way. Making it one token gives us a bit more flexiibility to change the operator precedence, but I don't see any particular reason to do that.

@michaelhkay michaelhkay added Tests Added Tests have been added to the test suites and removed Tests Needed Tests need to be written or merged labels Apr 21, 2024
@ChristianGruen
Copy link
Contributor

Minor detail: should "?[" be one token or two? I can't see any strong arguments either way. Making it one token gives us a bit more flexiibility to change the operator precedence, but I don't see any particular reason to do that.

I have no preference either.

I’m convinced that I would use the new syntax once it existed… but I can’t quite get used to the fact that we treat arrays and maps differently (this runs counter to our efforts to standardize the two data structures).

Next, while the classical filter expression is comparatively clean, we now run the risk of doing a lot of special casing. For expressions like…

$map-or-array?[. = 123]
$map-or-array?[?key = 123]

…there will be no chance to raise static errors.

@ndw
Copy link
Contributor

ndw commented Apr 23, 2024

The CG agreed to merge this issue at meeting 074

@ndw ndw merged commit 9f3fdc6 into qt4cg:master Apr 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A change that introduces a new feature Tests Added Tests have been added to the test suites
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants