JuliaLang / JuliaSyntax.jl

The Julia compiler frontend
Other
266 stars 32 forks source link

Update `Base.peek` docstring #440

Closed nhz2 closed 2 weeks ago

nhz2 commented 2 weeks ago

When looking at the help for peek in the repl, the current docstring looks like it applies to all IO, so I added ::ParseStream to make the docstring more specific. See also https://github.com/JuliaLang/julia/issues/54749

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 95.83%. Comparing base (bd29025) to head (3dca95e).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #440 +/- ## ========================================== - Coverage 96.17% 95.83% -0.35% ========================================== Files 14 13 -1 Lines 4185 3937 -248 ========================================== - Hits 4025 3773 -252 - Misses 160 164 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GunnarFarneback commented 2 weeks ago

This makes it clear that it's not an IO method of peek but most Base users won't have any idea what a ParseStream is and that they need to look it up as Base.JuliaSyntax.ParseStream.

But I think the real problem here is that JuliaSyntax shouldn't add a method to Base.peek at all but use a module local function for its internal ParseStream peeking.

KristofferC commented 2 weeks ago

Yes, either we remove the docstring or make it module local. No need for this to show in docs at all.

pfitzseb commented 2 weeks ago

Both of those seem like bad options, no? Base.peek is the right concept, so an overload seems appropriate. Removing the docstring seems weird as well.

Can't we do this level of filtering in Documenter.jl instead?

KristofferC commented 2 weeks ago

Okay, how about detecting when the package is put into the sysimage and not add the docstring in that case?

Also, if peek is the right concept, why does it need a docstring? The generic one should do in that case? I don't add docstrings to all my getindex methods.

pfitzseb commented 2 weeks ago

why does it need a docstring?

Because it has additional kwargs that should be documented.

Okay, how about detecting when the package is put into the sysimage and not add the docstring in that case?

Ok, so I think there are a few desirable properties for docstrings added by stdlibs:

  1. Documentation can be retrieved when the stdlib is loaded (by any mechanism, sysimg or no)
  2. stdlib docs are distinct enough from Base docs when viewed in the REPL (e.g. by printing the defining module)
  3. Documenter should separate/filter docstrings based on where they're defined (so the Base section only talks about Base methods)

Most of that holds for generic packages as well, and this PR is probably not the right place to talk about that anyways.


I think I'll just go ahead and merge this as-is, because it's an improvement. Further steps can be taken in a new PR.