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

Input functions should take Option #646

Merged
merged 2 commits into from
Sep 21, 2022
Merged

Input functions should take Option #646

merged 2 commits into from
Sep 21, 2022

Conversation

willmurnane
Copy link
Contributor

This PR contrasts with PR 641. The primary motivation here is that the various input functions take optional arguments, and produce optional results. This is implemented by changing the interface in pgx/src/inoutfuncs.rs, then the macro in pgx-macros/src/lib.rs, then teaching the SQL generator that this is acceptable in pgx/src/datum/mod.rs. Finally, I changed all the implementations of those functions that I could find, so that cargo test --workspace passes with these changes.

@workingjubilee
Copy link
Member

I think this needs at least one test to verify that the NULL -> NULL and NULL -> error message cases both work correctly. I am hoping the former is already effectively tested for (haven't checked yet) so it would be the latter, mostly. The latter just to make sure that the Rust &str we return from these functions is correctly used by Postgres during erroring.

@willmurnane
Copy link
Contributor Author

I added a pair of tests, and fixed up the interface in a minor way (using a constant rather than a function, so that implementors can override it).

@workingjubilee
Copy link
Member

Well, an implementation can override a default impl of a function too, no? I think it's mostly about ergonomics and if e.g. we would want to offer dynamic messages (I can't really think of why or how we would, however, since it's just a simple in/out).

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

This fixes some erroneous behavior and is fully compatible with #615. If any final retouch edits disrupt it I will resolve them myself, though I am still going to land #615 first.

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Looks like this passes tests!

@workingjubilee workingjubilee merged commit f6ae93f into pgcentralfoundation:develop Sep 21, 2022
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