DocStringExtensions.jl icon indicating copy to clipboard operation
DocStringExtensions.jl copied to clipboard

Implement `TYPEDSIGNATURENORETURN`

Open ReubenJ opened this issue 1 month ago • 12 comments

Addresses #159 following the suggestion at https://github.com/JuliaDocs/DocStringExtensions.jl/issues/159#issuecomment-1749488978.

ReubenJ avatar Oct 30 '25 13:10 ReubenJ

Thanks @ReubenJ, could I request though that the formatting changes aren't included to make the diff easier to review.

MichaelHatherly avatar Oct 30 '25 13:10 MichaelHatherly

Certainly! I also wanted to add another test or two before review. Thanks for the speedy reply!

ReubenJ avatar Oct 30 '25 13:10 ReubenJ

Pesky, pesky formatting. Should be good to go now @MichaelHatherly.

ReubenJ avatar Oct 30 '25 17:10 ReubenJ

Perhaps first #178 should be merged and then CI re-run here?

fingolfin avatar Oct 30 '25 17:10 fingolfin

Also, not sure what your release workflow is—should I include a (patch) version bump in this PR?

ReubenJ avatar Oct 30 '25 17:10 ReubenJ

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.

MichaelHatherly avatar Oct 30 '25 17:10 MichaelHatherly

Oops, something odd is happening, looks like the tests are not extensive enough yet. Give me a moment here.

ReubenJ avatar Oct 30 '25 17:10 ReubenJ

With the following docstring

"""
    $(TYPEDSIGNATURESNORETURN)

"""
function get_solver(pi::ProgramIterator)
        return pi.solver
end

I'm getting

search: get_solver

  get_solver(pi)
  

  get_solver(pi::ProgramIterator)

I wouldn't expect to see the untyped signature there. With TYPEDSIGNATURES, all I see is:

help?> get_solver
search: get_solver

  get_solver(pi::ProgramIterator) -> Any

ReubenJ avatar Oct 30 '25 17:10 ReubenJ

Stumped on this for the moment, I'll have to come back to it tomorrow.

ReubenJ avatar Oct 30 '25 18:10 ReubenJ

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 ReferenceTests.jl. I found it much easier to work with for this use case, though I'm happy to revert this change if you don't want to add a test dep.

ReubenJ avatar Nov 01 '25 06:11 ReubenJ

though I'm happy to revert this change if you don't want to add a test dep.

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.

MichaelHatherly avatar Nov 04 '25 11:11 MichaelHatherly

Sure! No problem.

ReubenJ avatar Nov 05 '25 12:11 ReubenJ

Reference tests are removed, and the branch is up to date. Any other changes you'd like to see for this to go through?

ReubenJ avatar Nov 07 '25 11:11 ReubenJ

Thanks @ReubenJ, let's get #180 before releasing a new version.

MichaelHatherly avatar Nov 25 '25 12:11 MichaelHatherly