Skip to content

Conversation

@c2main
Copy link

@c2main c2main commented Aug 17, 2025

code, test and doc overall ok, though there are some other references to functions_are() that I didn't explore yet.

And add doc for `procedures_are()`
c2main added 5 commits August 17, 2025 16:33
copy pasted with minor edition from `functions_are()`
It is now excluding procedures from the list of functions.
See `procedures_are()` instead.
@c2main c2main force-pushed the add-procedures_are branch from ce21421 to 2dcd43b Compare August 17, 2025 15:36
@hettie-d
Copy link

Thank you!

Copy link
Owner

@theory theory left a comment

Choose a reason for hiding this comment

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

Very thorough, but I think we should consider a deprecation period and a warning. Thoughts?

'functions',
ARRAY(
SELECT name FROM tap_funky WHERE schema = $1
SELECT name FROM tap_funky WHERE schema = $1 and kind != 'p'
Copy link
Owner

Choose a reason for hiding this comment

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

What do we do about people who have used these functions for procedures? With this change, their tests will start failing.

I wonder if maybe we should introduce a warning with a deprecation notice and instructions for them to update their tests when kind = 'p', and then remove it in a year or so.

Copy link
Author

Choose a reason for hiding this comment

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

see alternative in the other 'tab' discussion

Copy link
Owner

Choose a reason for hiding this comment

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

The migration file is missing the changes to the functions_are() functions.

@c2main
Copy link
Author

c2main commented Dec 12, 2025

Very thorough, but I think we should consider a deprecation period and a warning. Thoughts?

Yes I completely agree, though at the same time I'm facing another one with triggers, so maybe better keep functions_are as it is and just add a new set with an extra attribute (first), like ... mmmh a char ?! functions_are('f',....)

@theory
Copy link
Owner

theory commented Dec 15, 2025

Oh! ISTR that I've been thinking about has_functions() as applying to all functions, and then one can do separate calls to is_procedure(), is_aggregate(), is_window() to determine each one specifically. But I also see that there is no is_function(), but there is has_function(). This is confusing!

This could use some normalization. I think, ideally, we'd have has_{thing}s() and is_{thing}() for each type of function. Seems do-able, though has_function() would be limited as you have it here. I think maybe that's okay? We can rename the current has_functions() to has_routines() (the docs say "routine" is the collective term). Maybe we can have has_functions() go with the 'f' as in this patch, but then, for some period, if it fails and it looks like there are routines other than functions, emit a warning/diagnostics that the behavior has changed and run the test again without the 'f'.

Too complicated?

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