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.

Sign up for GitHub

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

Update for Documenter.jl v1 #683

Merged
merged 5 commits into from
Jan 9, 2024
Merged

Update for Documenter.jl v1 #683

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3050599
ignore `docs/Manifest.toml`
hyrodium Jan 4, 2024
6e77ab9
add missing docstrings
hyrodium Jan 4, 2024
e4de4ad
add compat table for Documneter.jl
hyrodium Jan 5, 2024
da73a0f
rename Other API with Internal API
hyrodium Jan 5, 2024
675b191
Use `checkdocs=:exports` and ignore internal docstrings as strongly s…
hyrodium Jan 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@
*.DS_Store
/docs/build/
/docs/site/
/docs/Manifest.toml
/benchmark_data/
/Manifest.toml
6 changes: 6 additions & 0 deletions docs/src/user/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,9 @@ ForwardDiff.GradientConfig
ForwardDiff.JacobianConfig
ForwardDiff.HessianConfig
```

## Other API
```@docs
ForwardDiff.can_dual
ForwardDiff.:≺
```
Copy link
Member

Choose a reason for hiding this comment

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

The docstring for can_dual is not useful at all, and both functions are not exported. IMO including them in the docs should be discussed and proposed in separate issues (#682) and PRs, but not be part of upgrading to Documenter v1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Documenter.jl v1 has warnonly=false keyword argument in makedocs. (JuliaDocs/Documenter.jl#2194), so makedocs will fail without adding the docs or setting warnonly=true.

These functions are already documented as docstrings, and I thought it would be okay to add them to the documentation. I understand these functions are not stable.
Should I replace "Other API" with "Internal API"?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK you can set checkdocs = :exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's right.
My point is, that the ignored docstrings may confuse other users. The functions are documented as docstrings, so someone may think these functions are stable. I thought emphasizing that these functions are not stable would help that kind of people.

Copy link
Member

Choose a reason for hiding this comment

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

But they are not exported, are they?

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I think these functions should not be included in the documentation for the time being and only added if they are considered important enough and their docstrings are more useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't quite understand.

Documentation should be written for both package users and developers, and including Internal API documentation will be useful for future package developers. Of course, if these docstrings are incorrect or meaningless, they should be removed, but that is not the case.

There are more functions and types that need to be written (#682), but I believe this is simply a matter of insufficient documentation, and not a reason to eliminate existing documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I am not the maintainer of the repository, I am open to following your guidance as the maintainer. If you believe that removing these documents is the best course of action, I am willing to proceed with that.

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT neither users nor developers should have to deal with can_dual and ≺, so even though there is a docstring I don't think it should be included in the documentation (to me it seems these are just more detailed comments). value, Dual, etc. are more relevant IMO: #682 (but this should be discussed and addressed separately)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll update this PR to remove them from the documentation.

I will write down additional my concerns for future reference:

  • If we use checkdocs=:exports, that will be as harmful as warnonly=true because this package provides non-exported stable API.
  • I agree that the docstrings are something like detailed comments. But it would be very hard to discuss whether new docstrings should be in the docs or not every time. The easiest way would be to document the all docstrings.

Loading