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 return types to Database class "pselect*" functions #4284

Closed
johnsaigle opened this issue Jan 28, 2019 · 6 comments
Closed

Add return types to Database class "pselect*" functions #4284

johnsaigle opened this issue Jan 28, 2019 · 6 comments
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation

Comments

@johnsaigle
Copy link
Contributor

#3878 did not add return types to pselect* functions because they can return array, null, and sometimes false (I believe). Wrapper functions in other classes and modules that call pselect functions are unable to have return types for the same reason.

It would be good to standardize and document the types that these functions can return and eventually to add appropriate return types.

@stale
Copy link

stale bot commented Feb 15, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the State: Stale PR that has had no recent activity, needs to be triaged for review or closure to proceed label Feb 15, 2020
@driusan
Copy link
Collaborator

driusan commented Feb 17, 2020

They shouldn't return false, they should all be either ?array or ?string, I think.

@stale
Copy link

stale bot commented Feb 17, 2020

The Stale label is being removed automatically because some activity has occurred or because the developers have decided that this pull request is important and should not continue to be overlooked.

@stale stale bot removed the State: Stale PR that has had no recent activity, needs to be triaged for review or closure to proceed label Feb 17, 2020
@johnsaigle
Copy link
Contributor Author

Agreed I think some of them have strange error handling though.

This issue is somewhat addressed by #6039 because it makes the array formats more clear. When that is merged we can take a look at making the functions more consistent.

@johnsaigle
Copy link
Contributor Author

I think for the most part they return ?array now. Only pselectOne() needs a return type. Adding it causes phan errors for downstream functions that use it though, so it will require a slightly larger cleanup PR.

@laemtl
Copy link
Contributor

laemtl commented Jan 8, 2021

It seems to have been addressed.

@laemtl laemtl closed this as completed Jan 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants