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

First and last #1067

Merged
merged 4 commits into from
Nov 25, 2024
Merged

First and last #1067

merged 4 commits into from
Nov 25, 2024

Conversation

bigkevmcd
Copy link
Contributor

@bigkevmcd bigkevmcd commented Nov 13, 2024

Pull Requests Guidelines

See CONTRIBUTING.md for more details about when to create
a GitHub Pull Request and when other kinds of contributions or
consultation might be more desirable.

When creating a new pull request, please fork the repo and work within a
development branch.

Commit Messages

  • Most changes should be accompanied by tests.
  • Commit messages should explain why the changes were made.
Add support for getting the first and last items from a list.

This adds support for doing something like `list.first()` and `list.last()` which return the first and last elements of a list respectively.

`.first()` is syntactic sugar, but `.last()` makes it simpler to get the last element from a list without having to compute the length of the list twice.

This can shorten a long expression instead of `nested.elements[nested.elements.size()-1]` you can rewrite this as `nested.elements.last().value()`.

Use of the optional functionality requires the registration of the `cel.OptionalTypes()` env option.

Reviews

  • Perform a self-review.
  • Make sure the Travis CI build passes.
  • Assign a reviewer once both the above have been completed.

Merging

  • If a CEL maintaner approves the change, it may be merged by the author if
    they have write access. Otherwise, the change will be merged by a maintainer.
  • Multiple commits should be squashed before merging.
  • Please append the line closes #<issue-num>: description in the merge message,
    if applicable.

This adds .first() and .last() to the list extension library which can
return the first and last elements from a list.

The results are returned as Optional values.

Signed-off-by: Kevin McDermott <[email protected]>
Make the lists first and last functions return optional optionals.

Signed-off-by: Kevin McDermott <[email protected]>
@seirl
Copy link
Collaborator

seirl commented Nov 17, 2024

Alternative suggestion, maybe supporting the negative indexing semantics from python (-1 == last, -2 == first before last...) would be another option. But it wouldn't have the optional return value when the list is empty, which can be nice too.

@TristonianJones
Copy link
Collaborator

TristonianJones commented Nov 18, 2024

@bigkevmcd you raise an interesting point regarding optional. I think my concern with having either null_type or optional_type(T) as a return type is that at first glance users won't know which it is. You can specify a cel.Lib as an cel.EnvOption, so it's possible to do any of the following things:

  1. Move first and last into another version of cel.OptionalTypes()
  2. Auto-enable optionals by providing cel.OptionalTypes() as an element in the CompileOptions() response
  3. Test whether optional types are enabled before adding the method signature

I think option 1 is the cleanest since the functions would be unambiguous in their signature and encourage the use of optional values. Option 2 runs into unexpected complexity being enabled, and option 3 runs into problems where cel.EnvOption order impacts which features are enabled (this is already a challenge with cel.CustomTypeProvider, and cel.ClearMacros)

@seirl
Copy link
Collaborator

seirl commented Nov 18, 2024

Alternative suggestion, maybe supporting the negative indexing semantics from python (-1 == last, -2 == first before last...) would be another option. But it wouldn't have the optional return value when the list is empty, which can be nice too.

I remembered _[?_]. So with negative indexing you could have:

  • list.first() -> list[?0]
  • list.last() -> list[?-1]

@TristonianJones
Copy link
Collaborator

@seirl while the use of a negative index would certainly be very clean, I'm worried that users who have implemented custom CEL lists will not be able to support the negative index without a rewrite of the expression to list.size() > 0 : optional.of(list[list.size()-1]) : optional.none()

@seirl
Copy link
Collaborator

seirl commented Nov 19, 2024

OK, I didn't realize this would be an issue, I expected that the negative index translation would happen before the custom list implementation.

I've also seen people use l.reverse()[?0] but it makes a copy of the list.

@TristonianJones
Copy link
Collaborator

/gcbrun

@bigkevmcd bigkevmcd force-pushed the first-and-last branch 2 times, most recently from 298d831 to 36121a5 Compare November 23, 2024 18:12
@TristonianJones
Copy link
Collaborator

/gcbrun

@TristonianJones
Copy link
Collaborator

@bigkevmcd, thanks for the contribution!

@TristonianJones TristonianJones merged commit 5d18e93 into google:master Nov 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants