-
Notifications
You must be signed in to change notification settings - Fork 34
Implement TYPEDSIGNATURENORETURN
#179
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks @ReubenJ, could I request though that the formatting changes aren't included to make the diff easier to review. |
|
Certainly! I also wanted to add another test or two before review. Thanks for the speedy reply! |
1902da1 to
2057bd6
Compare
The abbreviation should behave in the same way as TYPEDSIGNATURE but omit the return type.
cdc9211 to
8a740b3
Compare
8a740b3 to
8f1797d
Compare
|
Pesky, pesky formatting. Should be good to go now @MichaelHatherly. |
|
Perhaps first #178 should be merged and then CI re-run here? |
|
Also, not sure what your release workflow is—should I include a (patch) version bump in this PR? |
Can be done separately to this one. |
|
Oops, something odd is happening, looks like the tests are not extensive enough yet. Give me a moment here. |
|
With the following docstring """
$(TYPEDSIGNATURESNORETURN)
"""
function get_solver(pi::ProgramIterator)
return pi.solver
endI'm getting search: get_solver
get_solver(pi)
get_solver(pi::ProgramIterator)I wouldn't expect to see the untyped signature there. With help?> get_solver
search: get_solver
get_solver(pi::ProgramIterator) -> Any |
|
Stumped on this for the moment, I'll have to come back to it tomorrow. |
|
No luck reproducing that behavior I saw while testing the changes in another repo, so I think this is ready to go as-is. In the process of trying to reproduce what I saw above, I switched the new tests to use |
Yes, please revert that. If we're going to use ReferenceTests for testing that's fine, and I'd like to see that change, but it would need to be applied to all tests as a single change set rather than adding a few new ones that use it. Best to keep that as a completely separate PR rather than as part of this feature addition. |
|
Sure! No problem. |
a91c808 to
8f1797d
Compare
Addresses #159 following the suggestion at #159 (comment).