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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add generated documentation for library functions #49275

Closed
wants to merge 5 commits into from

Conversation

@tazjin
Copy link
Member

@tazjin tazjin commented Oct 27, 2018

This adds generated documentation using nixdoc for some of the library functions in <nixpkgs>.lib. It is part of my NixCon 2018 hackathon project.

A rendered view is available for your viewing pleasure. 馃憖

This PR previously included the changes to documentation comments, but those have been moved to #49383, see this comment for why.

The documentation files can be generated by installing nixdoc (available in master) and running:

# in nixpkgs/doc:
nixdoc -c 'strings' -d 'String manipulation functions' -f ../lib/strings.nix  > functions/library/strings.xml
nixdoc -c 'trivial' -d 'Miscellaneous functions' -f ../lib/trivial.nix  > functions/library/trivial.xml
nixdoc -c 'lists' -d 'List manipulation functions' -f ../lib/lists.nix  > functions/library/lists.xml
nixdoc -c 'debug' -d 'Debugging functions' -f ../lib/debug.nix > functions/library/debug.xml
nixdoc -c 'options' -d 'NixOS / nixpkgs option handling' -f ../lib/options.nix  > functions/library/options.xml

Some notes & caveats:

  • functions returning other functions via currying currently have incorrect function argument annotations, as there is no easy way to distinguish between arguments for the returned function and arguments for the library function
  • the manual index becomes pretty large with these functions included. I discussed with @grahamc that it may be sensible to move these function docs to an appendix
  • due to the manual override functionality implemented after tazjin/nixdoc#14 the manual build prints a whole bunch of warnings for missing files. There may be a way to suppress those ...
@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 27, 2018

Oh, and before I forget it - shoutout to @jD91mZM2 for writing rnix, which made this relatively easy to implement by preserving comments in the AST representation!

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 27, 2018

Updated with lib.lists. This is about as much as I can get done on this laptop keyboard!

@joachifm
Copy link
Contributor

@joachifm joachifm commented Oct 28, 2018

Very nice :) Is it feasible to generate the docs during the build instead of checking them into the repo?

@grahamc
Copy link
Member

@grahamc grahamc commented Oct 28, 2018

Unfortunately not, @joachifm -- nixdoc requires unstable rust features which we can't get without using the beta rust compiler, and we almost certainly don't want to make that a blocker for releasing.

This looks fabulous -- way to go @tazjin!

@tazjin tazjin force-pushed the tazjin:docs/lib-docs branch Oct 28, 2018
@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 28, 2018

@joachifm Eventually we'd like to do that, but as Graham mentioned the nixdoc tool currently depends on nightly Rust because one of the dependencies (the Nix parser, in fact) does.

The required features are going to stabilise not too far in the future, at which point we could add it to the automatic manual generation. An alternative approach is removing the use of unstable features from that lib and its dependencies, which @Mic92 has started doing.

Either way, for now just checking in the generated files already gets us quite far!

@jD91mZM2
Copy link
Member

@jD91mZM2 jD91mZM2 commented Oct 28, 2018

I believe Rust 2018 edition is, if all goes well, coming in 1.31 (the very next update of rust) which should get rid of the nightly dependency.

EDIT: Did it anyway, a backported version is now available on the stable branch :)

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 28, 2018

@jD91mZM2 Awesome, thank you! Would you consider publishing the stable-compatible rnix and arenatree to crates.io?

It seems like rustPlatform.buildRustPackage chokes on git-dependencies and carnix chokes on Gitlab repositories (it shan't be easy!).

Edit: Nevermind, managed to patch carnix locally to work with the git repository.

@jD91mZM2
Copy link
Member

@jD91mZM2 jD91mZM2 commented Oct 28, 2018

I'll publish them as soon as I get a PR to arenatree merged, to avoid publishing twice. Will let you know when (in a day or two)

tazjin added a commit to tazjin/nixpkgs that referenced this pull request Oct 28, 2018
Adds nixdoc, a tool to generate documentation for Nix functions in the
standard library.

See NixOS#49275 for some background information.
@tazjin tazjin mentioned this pull request Oct 28, 2018
3 of 9 tasks complete
@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 28, 2018

As you can see above there's now a PR for adding nixdoc to nixpkgs using stable Rust. I still would prefer to get pre-generated docs checked in now, so we can deal with building a fully automated pipeline later on.

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Oct 28, 2018

@tazjin you should be able build git dependencies now with buildRustPackage since #46362

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 28, 2018

@Mic92 I used buildRustCrate now and it works fine.

@tazjin tazjin requested a review from Profpatsch as a code owner Oct 28, 2018
@Mic92 Mic92 mentioned this pull request Oct 28, 2018
0 of 9 tasks complete
@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 28, 2018

Added docs for lib.options and lib.debug on the flight back to Oslo :)

There are some slightly unclear things in both of those, so it'd be appreciated if someone could do a second pass - but we can do that in a followup PR.

Mic92 added a commit to Mic92/nixpkgs that referenced this pull request Oct 28, 2018
Adds nixdoc, a tool to generate documentation for Nix functions in the
standard library.

See NixOS#49275 for some background information.
@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Oct 29, 2018

@tazjin is this ready to merge?

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 29, 2018

It's ready technically, I'll just make more PRs when I cover the other files.

Two notes:

  1. The manual index becomes quite large after merging, @grahamc suggested that we could move it to an appendix - I don't know how that works though.

  2. It'd probably be nice if someone read over the doc strings I modified and gave a thumbs up/down, but those are also easy to improve upon later.

@grahamc
Copy link
Member

@grahamc grahamc commented Oct 29, 2018

What do you think about this strategy for merging:

  1. split this in to two PRs: one for the .nix file updates, and one for the generated docs
  2. backport the .nix updates to 18.09
  3. create a new PR for docs generation for master
  4. create a new PR for docs generation for 18.09 (generated separately, I think?)

Ideally the "update generated docs" step would be a make target in the Makefile, not called by default.

Before merging the generated docs, I'd like to do a rendered display for people to look at.

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 29, 2018

Before merging the generated docs, I'd like to do a rendered display for people to look at.

Oh, I forgot to mention this - I publish rendered docs here at the moment.


split this in to two PRs: one for the .nix file updates, and one for the generated docs

Works for me. I'd make a new PR for the .nix updates and extract them out of this one. I'd keep this as the PR for docs generation in master.

For the 18.09 PR I suppose the steps are something like:

  1. Check out 18.09 branch.
  2. Generate docs in there.
  3. PR to that branch.

Correct? Or is that branch no longer in use?


Ideally the "update generated docs" step would be a make target in the Makefile, not called by default.

Lets do this as the next step, we can discuss a little bit what the best workflow is but for now I just want to get Some Docs 鈩笍 in.

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 29, 2018

Changes to Nix-files have been removed from this PR, they are now in #49383.

@Profpatsch Profpatsch removed their request for review Oct 29, 2018
@grahamc
Copy link
Member

@grahamc grahamc commented Oct 29, 2018

Working on making it an appendix. One maybe long-term project is to fix the code examples with absolute function names, so

mod 11 10
=> 1
mod 1 10
=> 1

becomes

lib.trivial.mod 11 10
=> 1
lib.trivial.mod 1 10
=> 1

this might be part of a larger effort to improve the examples, though, like adding titles and splitting shared examples in to two?

Also, what is the status of splitting the input with the output, for testable doc comments?

@tazjin
Copy link
Member Author

@tazjin tazjin commented Oct 29, 2018

One maybe long-term project is to fix the code examples with absolute function names

You mean by actually updating the doc comments to look like that, or via some spooky rewriting?

Also, what is the status of splitting the input with the output, for testable doc comments?

It depends on me writing a proper parser for the doc comments. Currently it's a somewhat advanced string-splitter that works well with the existing format, but it should be a bit more flexible. I'll probably start looking into it this evening.

@Ekleog
Copy link
Member

@Ekleog Ekleog commented Dec 12, 2018

Note: once merged, I think this will obsolete #23505

@matthewbauer matthewbauer requested a review from grahamc Dec 15, 2018
tazjin added 5 commits Oct 27, 2018
Adds documentation generated with [nixdoc][] for the functions in
`trivial.nix`.

There are a couple of minor issues:

* Some markup syntax used in comments is not recognised and rendered
  literally. This isn't so much a bug, as a design decision.

* Functions *returning* functions have incorrect function parameter
  annotations because there is no way to distinguish between the
  arguments of the functions when currying.

[nixdoc]: https://github.com/tazjin/nixdoc
Add nixdoc-generated documentation for the functions in lib.strings.

The new file should not be modified manually.
Add nixdoc-generated documentation for the functions in lib.lists.

The new file should not be modified manually.
@tazjin tazjin force-pushed the tazjin:docs/lib-docs branch from e0a6ba9 to 98ba629 Dec 29, 2018
@tazjin
Copy link
Member Author

@tazjin tazjin commented Dec 29, 2018

I've rebased this and regenerated all the docs above (there were a handful of new functions).

@grahamc When do you think we can get this merged?

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 29, 2018

This looks awesome! One suggestion I have is to put the commands to regenerate these files somewhere in the repo. Maybe also somehow make sure that they get run before a channel update.

@tazjin
Copy link
Member Author

@tazjin tazjin commented Dec 29, 2018

It should be possible to build a setup that does this as part of the manual build, but for now I mostly just want to get some version of them in as a starter ;-)

@Infinisil
Copy link
Member

@Infinisil Infinisil commented Dec 29, 2018

I'd be fine with just a simple Readme.md with the instructions you have at the top of this PR for now, nothing too sophisticated. Just so this is captured somewhere and people don't need to find this PR to update it.

@tazjin
Copy link
Member Author

@tazjin tazjin commented Dec 30, 2018

An alternative approach is in this commit. Instead of checking in the generated files it updates the manual build process to generate them, which is not too difficult.

Currently due to the way this works the list of files that needs to be documented is specified twice (once for generation of the doc XML, and once in another XML file to include the generated files). I think this is a very small price to pay for now though. Thoughts?

@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Dec 31, 2018

I think I prefer the other pr.

@tazjin
Copy link
Member Author

@tazjin tazjin commented Dec 31, 2018

@Mic92 Agree, and @Infinisil does, too. I'm closing this PR in favour of the other one.

@tazjin tazjin closed this Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants