-
Notifications
You must be signed in to change notification settings - Fork 161
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
fix: remove function definitions w/ invalid return types #599
Conversation
T? and T|? are not valid return types T|? is equivalent to setting the nullability to MIRROR T&? is not currently expressible via nullability BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed
Here's the PR that introduced those for reference: #247 |
@richtia thanks for linking that 🙇 Additional context from @westonpace in Slack
Part of the discussion in Slack was also around adding a new nullability type like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approve if we want to proceed with this. However, would it be better to keep the methods around and just change the return type to T
? Is "slightly inaccurate but usable" better than "accurate but not usable"?
T? and T|? are not valid return types T|? is equivalent to setting the nullability to MIRROR T&? is not currently expressible via nullability BREAKING CHANGE: least_skip_null and greatest_skip_null functions have updated return types
@westonpace I think it makes sense to keep them available so that they can be used. In this case, using nullability MIRROR is weaker than the actual nullability desired so it seems safe. Made a change to bring them back in, update the return types and include a note about it. I've created #601 as an issue to track this new desired nullability as well. |
If @EpsilonPrime is still good with the updated version I think we are good to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks.
Note, this part of the description is not true anymore:
BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed
Will merge this as this has 3 SMC votes (including myself) |
T? and T|? are not valid return types
T|? is equivalent to setting the nullability to MIRROR
T&? is not currently expressible via nullability
BREAKING CHANGE: least_skip_null and greatest_skip_null functions have been removed