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

Docs: TwicePrecision is an internal type #42863

Merged
merged 2 commits into from
Nov 4, 2021
Merged

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 30, 2021

Fixes #42857

CC @imyxh

The trouble of using the browser...
@KristofferC
Copy link
Member

KristofferC commented Oct 30, 2021

Tangential gripe, but this is one issue I have with putting normal docstrings on basically anything with the justification that "more docs can't hurt, right?". Well, it becomes very hard for someone who does exploration through the REPL to know what is internal and what is not since you basically have to look through the whole manual to see that the thing is not documented there, and is therefore internal. Manually adding a big warning that something is internal for every docstring feels kinda clunky as well.

It would be good if the manual and the REPL could "cooperate" to automatically print something when one looks at an internal docstring.

@timholy
Copy link
Member Author

timholy commented Oct 30, 2021

I like that perspective. But in cases where you want to educate people into the "right" way to do it, what do you see as the alternative to a hand-written warning?

@Seelengrab
Copy link
Contributor

I don't like that perspective because it significantly increases the barrier to entry for contributing to internal/Base functions to investigate/fix a bug (which is almost always necessary not to accidentally break something else). It's either uncertainty about what things should behave like or having to spend hours just to figure out what a function is supposed to do/provide as a contract to other internal functions when trying to fix a legitimate bug.

To me the issue is that (as far as I know) there's no clear, prominent guideline for what really is considered API provided by Base, not even on a per-function level (not to mention behavior or stdlibs). This imo has nothing to do with being exported/documented or not. These things live on three independent axis.

It would be good if the manual and the REPL could "cooperate" to automatically print something when one looks at an internal docstring.

How should the REPL know what is an "internal docstring" when a human looking at it in isolation doesn't know it? Why should I have to go through an indirection through the REPL when e.g. reading some source file to figure out what it does?

@timholy
Copy link
Member Author

timholy commented Nov 1, 2021

@Seelengrab, the criterion is: does it appear in https://docs.julialang.org/en/v1/? (I forget where that's written down, but it is somewhere.) The API lists there are generated manually, so it was a matter of choice.

The way I took @KristofferC's comment is not that we shouldn't document internal functions, but that we should take an additional step: for any docstring that doesn't appear in the @doc lists, have the REPL automatically print a warning that this is not part of Julia's public interface. I think that resolves all your concerns.

@Seelengrab
Copy link
Contributor

Seelengrab commented Nov 1, 2021

No, your comment highlights exactly what I think is wrong. You're a core contributor and not even you can figure out where it's documented what is considered API (which isn't fully documented btw - #31202 has been stalled for quite some time and only covers exported functions. I tried to tackle that recently, but the script for checking which exported thing is undocumented broke and I'm not well versed enough in internals to fix it). How is someone wishing to contribute either new docs (which is often said to be the easiest PR to make..) or a new feature supposed to know how/where to add a magic @doc annotation to make it show up in the manual? Are there cases where something isn't exported, but considered API anyway? I can think of one immediately - Base.Threads still isn't exported (at least using Threads doesn't work), but it certainly does have an entry in the docs. There may be more instances like this, even at the function level. Why not have the marker for internal vs. not internal right where it belongs - at the function definition/the relevant docs?

for any docstring that doesn't appear in the @doc lists, have the REPL automatically print a warning that this is not part of Julia's public interface. I think that resolves all your concerns.

No, I don't think it does. It at least doesn't address my concern regarding reading through source files and figuring out whether something is internal or not (though granted, at the moment it's way more likely to have any given function be internal when digging through julia source). If possible, I'd like to avoid special casing in the REPL/manual building.


One way to solve this could be through the admonition system of our Markdown implementation (I think Documenter inherited that to some degree, right?) - we could have a !!! API admonition that explicitly states "this is considered external API you can rely on when developing your code - save for bugfixes, this function may only change in the next major version, but not minor or patch version". The default for any docstring that doesn't have this admonition (possibly even functions without a docstring at all?) would be to add a different admonition at the end of the docstring e.g !!! Internal, meaning "this is considered internal and may change or be removed in any new version of julia". This is most likely a place where this approach needs work, since we can't assume in the REPL that functions outside of base that don't have a !!! API admonition are internal.
However, this could even extend to deprecation notices - at the moment, I don't think the @deprecate macro adds docs to deprecated methods, but it could add a !!! Deprecated admonition with text similar to "This method is deprecated and will be removed in the next major/minor/x version", with optionally "use the other version instead" in case an alternative was given. This way, new contributors don't have to mess with REPL or manual building internals and can focus on actually writing and improving documentation.

The obvious upside is that even internal and deprecated functions are browsable through both the manual (perhaps with seperate sections for API and internal functions? Can Documenter create a section based on the existence of an admonition in the docstring?) and REPL, as well as making it obvious in the source code what API state a function is in. The even bigger upside is that internal functions are incentivised to be documented as far as their behavior goes - they'd be documented internal by default after all. At the moment it's just too easy to not document at all, since no one will notice anyway unless they try to fix something and have to reverse engineer the code (for example: #41649) and having a docstring may or may not lead to it showing up in the manual. This makes it much harder than necessary to just dive in and hack on/fix a bug.


Apologies for highjacking your PR though - should I open an issue and move the general discussion there?

@KristofferC
Copy link
Member

KristofferC commented Nov 1, 2021

? Are there cases where something isn't exported, but considered API anyway? I can think of one immediately -

Of course, Meta.parse, Profile.print etc.

At the moment it's just too easy to not document at all, since no one will notice anyway unless they try to fix something and have to reverse engineer the code (for example: #41649)

That should have had a comment, not a docstring.

Copy link
Member

@simeonschaub simeonschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. The discussion here seems mostly tangential, so I think this can just be merged.

@timholy timholy merged commit d00d457 into master Nov 4, 2021
@timholy timholy deleted the teh/twiceprec_is_internal branch November 4, 2021 23:08
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

length(::TwicePrecision) not defined leads to broadcasting failing
4 participants