Skip to content

First and last#1067

Merged
TristonianJones merged 4 commits into
cel-expr:masterfrom
bigkevmcd:first-and-last
Nov 25, 2024
Merged

First and last#1067
TristonianJones merged 4 commits into
cel-expr:masterfrom
bigkevmcd:first-and-last

Conversation

@bigkevmcd

@bigkevmcd bigkevmcd commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

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 <bigkevmcd@gmail.com>
Make the lists first and last functions return optional optionals.

Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>
@seirl

seirl commented Nov 17, 2024

Copy link
Copy Markdown
Collaborator

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

TristonianJones commented Nov 18, 2024

Copy link
Copy Markdown
Collaborator

@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

seirl commented Nov 18, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

seirl commented Nov 19, 2024

Copy link
Copy Markdown
Collaborator

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.

Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>
@TristonianJones

Copy link
Copy Markdown
Collaborator

/gcbrun

Comment thread cel/library.go
Comment thread cel/library.go
Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>
@TristonianJones

Copy link
Copy Markdown
Collaborator

/gcbrun

@TristonianJones

Copy link
Copy Markdown
Collaborator

@bigkevmcd, thanks for the contribution!

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