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 empty optional fields-aware record operator variants #1876

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

yannham
Copy link
Member

@yannham yannham commented Mar 28, 2024

The design guideline around empty optional fields has been to make most normal record operation ignore them, to avoid issues like trying to list the field of a record and access them just to fail on an empty optional introduced by a contract, which we shouldn't care about.

However, in some cases (typically operating on record contracts), users might want to actually take empty optional fields into account. Most primops were already existing in two variants internally (ignore empty opts and consider all fields). This PR just makes them available as clearly documented stdlib functions, and update the documentation of the existing record functions to make their behavior with respect to empty optionals more explicit.

Naming

I use the _all prefix for now, but I don't find it optimal. insert_all and remove_all looks like they're taking several fields to insert and remove. I'm open to suggestions. An alternative could be extd for extended. It's not very telling, but at least doesn't set up false expectations.

The design guideline around empty optional fields has been to make most
normal record operation ignore them, to avoid issues like trying to list
the field of a record and access them just to fail on an empty optional
introduced by a contract, which we shouldn't care about.

However, in some cases (typically operating on record contracts), users
might want to actually take empty optional fields into account. Most
primops were already existing in two variants internally (ignore empty
opts and consider all fields). This commit just makes them available as
clearly documented stdlib functions.
@yannham yannham requested review from jneem and vkleen March 28, 2024 11:24
@github-actions github-actions bot temporarily deployed to pull request March 28, 2024 11:27 Inactive
@yannham yannham changed the title Feat/record ops variants Add empty optional fields-aware record operator variants Mar 28, 2024
Copy link
Member

@jneem jneem left a comment

Choose a reason for hiding this comment

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

LGTM. I think with_opts was the suffix we settled on in the weekly.

@yannham
Copy link
Member Author

yannham commented Mar 29, 2024

During the renaming _all -> _with_opts, I realized I forgot to add has_field_with_opts as well.

The previous commit introducing variants of record operators that don't
ignore empty optional fields missed `has_field`, whose variant is added
by this commit.

As decided by the team in the weekly meeting, the `_all` suffix
introduced in the previous commit has been changed to `_with_opts`,
which is a bit longer but less confusing and more explicit.
@github-actions github-actions bot temporarily deployed to pull request March 29, 2024 10:09 Inactive
@yannham yannham added this pull request to the merge queue Mar 29, 2024
Merged via the queue into master with commit b2e7713 Mar 29, 2024
5 checks passed
@yannham yannham deleted the feat/record_ops_variants branch March 29, 2024 14:03
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