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

Add optional.unwrap() / .unwrapOpt() function #1103

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

seirl
Copy link
Collaborator

@seirl seirl commented Jan 10, 2025

This function takes a list of optional values and only returns the non-none values from the list, skipping the none values, and returning a list of the unwrapped values directly.

@seirl seirl requested a review from TristonianJones January 10, 2025 14:37
@TristonianJones
Copy link
Collaborator

@seirl we ended up choosing a different name for this feature, with optional.unwrap in the cel-java repo: google/cel-java#438

I think the behavior is the same, but would you mind taking a look and updating to match?

@seirl
Copy link
Collaborator Author

seirl commented Jan 13, 2025

Ah, okay. I think it might hinder readability in our case, because we usually have long chains of map/filter/flatten/elideOpt/all/exists/....
The fact that all of these are postfix except optional.unwrap() is going to make it less readable, as readers will have to go back and forth in the line.
Is this still something that could be changed or is it too late?

@TristonianJones
Copy link
Collaborator

Hi @seirl, my impression is that the chains will continue to work just fine, but you'd probably just want to wrap the end result into an optional.unwrap to make sure a non-empty list of values is produced. A few examples might help in the event that I'm misunderstanding the use case.

@seirl
Copy link
Collaborator Author

seirl commented Jan 14, 2025

The problem is that mixing up prefix and postfix is less readable in long processing chains:

mylist
  .sortBy(foo, foo.bar)
  .elideOpt()
  .filter(x, x.value < 42)
  .elideOpt()
  .flatten()

vs:

optional.unwrap(
  optional.unwrap(
    mylist
      .sortBy(foo, foo.bar)
  ).filter(x, x.value < 42)
).flatten()

(which I had to re-read 3 times to make sure that it was correctly parenthesized)

So my preference is for postfix because it's the most consistent, but we could also have both for backwards compatibility (one prefix and one postfix, maybe .unwrapOpt()?).

@TristonianJones
Copy link
Collaborator

Consider, for a moment, that the expression mylist.sortBy(foo, foo.bar) won't work since optionals are not comparable, nor would a filter expression work when the list is not already flattened.

My impression of what you're actually going to have to support is something more like the following:

optional.unwrap(mylist).sortBy(foo, foo.bar).filter(foo, foo.value < 42)

That is to say, I expect the unwrap to typically happen at the beginning or end of an expression chain. If there are more examples of when we might perform intermediate computations, I'm willing to consider a chained unwrap which takes a list target value.

@seirl
Copy link
Collaborator Author

seirl commented Jan 15, 2025

Right, sorry! Here is a more realistic example partially taken from our codebase:

lists.range(size(foo.target))
  .filter(i, foo.target[i].?attr.?attr.orValue("").startsWith("/foobar/"))
  .map(i, foo.qux[i])
  .elideOpt()
  .sortBy(x, x.blah)
  .distinct()

@TristonianJones
Copy link
Collaborator

@seirl thank you, that helps me better understand. In that case, to be consistent, let's introduce optional.unwrap (namespaced method) and <list(optional(T))>.unwrapOpt() -> list(T)

@seirl seirl changed the title Add .elideOpt() function Add .unwrapOpt() function Jan 22, 2025
This function takes a list of optional values and only returns the
non-none values from the list, skipping the none values, and returning a
list of the unwrapped values directly.
@seirl seirl changed the title Add .unwrapOpt() function Add optional.unwrap() / .unwrapOpt() function Jan 23, 2025
@seirl
Copy link
Collaborator Author

seirl commented Jan 23, 2025

@TristonianJones PTAL :-)

@TristonianJones TristonianJones merged commit 7621362 into google:master Jan 23, 2025
2 checks passed
@seirl seirl deleted the elide_opt branch January 24, 2025 15:21
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.

2 participants