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

nix repl: Provide documentation from comment when evaluating to lambda #1652

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@roberth
Copy link

roberth commented Oct 31, 2017

This provides limited support for python-like docstrings in the nix repl. When the user evaluates an expression to a lambda, nix repl will now print the contents of a documentation comment, as long the comment written right before the attribute, and the attribute value is an actual lambda.

Current limitations:

  • Overriding the documentation requires redundant lambdas (eta abstraction to be precise)
  • No docstring support for anything other than functions

Demo:

nix-repl> pkgs.lib.concatMapStringsSep ":"
«lambda @ /nix/store/ksi72626r14035xkndbn584pmb7l703r-nixos-17.09.1535.1fdca25ee8/nixos/lib/strings.nix:64:30»

| concatMapStringsSep
| -------------------
| 
| NOTE: This function has already been applied!
|       You should ignore the first 1 parameter(s) in this documentation,
|       because they have already been applied.
|
| First maps over the list and then concatenates it.
| 
| Example:
|    concatMapStringsSep "-" (x: toUpper x)  ["foo" "bar" "baz"]
|    => "FOO-BAR-BAZ"


nix-repl>

Changes:

  • A new test set, nix-repl.sh
  • A new module, comment.cc for the documentation retrieval logic
  • Additions to repl.cc

Todo:

  • choose a comment syntax
  • hide the documentation behind a hint that :doc displays the documentation for the function
@regnat

This comment has been minimized.

Copy link
Contributor

regnat commented Nov 1, 2017

Really nice improvement :)

It may be more natural to have this printed only with the :t command (or maybe a new :doc one) rather than when evaluating, what do you think of this?

@grahamc

This comment has been minimized.

Copy link
Member

grahamc commented Nov 1, 2017

@gilligan

This comment has been minimized.

Copy link

gilligan commented Nov 1, 2017

Oops.. i didn't realize the PR was already open and commented on your fork instead.. well you can see it anyway ;)


std::string rawComment = matches[1];
std::string name = matches[2];
int timesApplied = countLambdas(matches[3]);

This comment has been minimized.

@gilligan

gilligan Nov 1, 2017

I think matches could be of length 3 here and you are accessing the 4th element? Since you check for < 3 in line 126..

}

// SLOW, probably O(n^2)
std::string stripPrefix(std::string prefix, std::string s) {

This comment has been minimized.

@gilligan

gilligan Nov 1, 2017

How about something like this:

std::string stripPrefix(std::string prefix, std::string s) {
  std::string::size_type index = s.find(prefix);
  return (index == 0) ? s.erase(0, prefix.length()) : s;
}

Untested but I guess that should work?! ;-)

regex_search(sourcePrefix, matches, e);

std::stringstream buffer;
if (matches.length() < 3) {

This comment has been minimized.

@gilligan

gilligan Nov 1, 2017

I really don't know what kind of standards the nix code base has in general.. i usually try to avoid "magic numbers" and would introduce some #define here or so.. same for the indices to access matches below. Just a thought.

@gilligan

This comment has been minimized.

Copy link

gilligan commented Nov 1, 2017

@roberth I wonder if this should perhaps restricted to not just any kind of /* .. */ delimited block but that we instead introduce some marker ?

{
  /*nixdoc
  Proofs that P = NP
  */
  proofThingy = p: np: 42;
}

Otherwise we might end up with arbitrary things like TODO: rewrite this or FIXME or whatever else. Since we (NixOsDocs folks) are starting off with improving nix functions documentation we could pick this up from the get go.

This would definitely also help for automatically extracting docstrings like these from nixpkgs. In fact - maybe we could go even further and use nixdoc:<function> (So nixdoc:proofThingy for the example above). That would make it really easy to create things like ctags files for navigating nixpkgs. I would love that.

@roberth roberth force-pushed the roberth:lambda-docstring branch 2 times, most recently from aa530fb to d05552b Nov 7, 2017

@roberth

This comment has been minimized.

Copy link
Author

roberth commented Nov 7, 2017

@gilligan , thank you for your review. I have implemented all of your suggestions except the nixdoc:.

I'm also a bit concerned about the ugly comments that might show up. I'm not sure what the special syntax should be. nixdoc:myfunc will inevitably lead to copy paste errors and bitrot. A 'small syntax' like /** */ might be better than nixdoc, but that leaves no room for metadata in the comment that isn't normally shown...

@gilligan

This comment has been minimized.

Copy link

gilligan commented Nov 7, 2017

@roberth right, having the function name duplicated in the comment might easily go out of sync. Then again: doesn’t that also apply to any kind of comment written for any function anyway?

So the function documentation can always be wrong but if we have some tag we could maybe limit the output to the “right” kind of comment blocks?

In short: IMHO looking for “/** nixdoc” might be preferable.

@roberth

This comment has been minimized.

Copy link
Author

roberth commented Dec 2, 2017

@gilligan I agree that we need some kind of marker to indicate that it's a documentation comment.
I think /** or ## should be sufficient, but it's also nice to be forward compatible, so we can switch to RST/Markdown/whatever when we want to generate HTML docs. My suggestion:

 * `/**` followed by plain text, to be shown verbatim in the repl
 * `/**sometoken ` followed by text in some format

We can remove the sometoken in the repl, for forward compatibility. When we're going to generate HTML docs, we can decide on a format without causing breakage when switching. So in the future, docs may look like

`/**md
 # Introduction

 This function bla bla
` */

What I like about this is that it is very clear that we're using markdown here, it will be shown in the repl without the md part, and can be processed in a future repl, knowing that it's the 'Markdown+Nix' format.

I'm not saying that we should do Markdown in the future, though. There's no standard for it. At least Restructured Text has a better specification (from what I've heard). Markdown is rather popular though.
Let's not go for docbook at least :)

@dtzWill

This comment has been minimized.

Copy link
Contributor

dtzWill commented Dec 20, 2017

Sounds good to me! Can't wait! ^_^

@roberth

This comment has been minimized.

Copy link
Author

roberth commented Jan 10, 2018

Before the holidays, I put out a twitter poll about the syntax for documentation comments. The results:

It got 24 votes:

33% /** always plain text / (8 people)
33% /
* always markdown */
17% /**md to pick md format (4 people)
17% ## let's discuss\n=======
One person mentioned doxygen, but didn't gain support from others.
On IRC, grahamc mentioned docbook

So, there is no clear winner, but a combination of /** plain text by default and /**md to 'upgrade' to markdown can be argued to have 50% of the votes (ignoring the possibility that some plain text voters hate markup languages at all cost)

If anyone is going to decide the color of this bike shed I think it should be @edolstra because I don't want to ruin his awesome creation with ugly comments.

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Jan 10, 2018

I'm not in favor of Markdown.

@roberth

This comment has been minimized.

Copy link
Author

roberth commented Jan 10, 2018

@edolstra good.

What about an extensible syntax marked by a keyword right after /**?
Or do you want to 'disallow' plain text (most comments now) and standardize on a single format? If so, which one? Some possibilities are restructured text (has a specification; python doc format), docbook and doxygen.


return parseDoc(buffer.str());
} catch (std::exception e) {
std::cout << "Caught exception: " << e.what() << std::endl;

This comment has been minimized.

@edolstra

edolstra Jan 10, 2018

Member

=> ignoreException().

i++) {
buffer << line << "\n";
}
buffer << line.substr(0, pos.column-1);

This comment has been minimized.

@edolstra

edolstra Jan 10, 2018

Member

This can be simplified to something like for (auto & line : tokenizeString(readFile(pos.file), "\n") { ... }.

This comment has been minimized.

@roberth

roberth Jan 11, 2018

Author

tokenizeString didn't work because it doesn't give back the empty lines. I didn't find another suitable function in util either, so I have factored the thing out instead. The other review items are now solved.

//
// Will return empty values if nothing can be found.
// For its limitations, see the docs of the implementation.
struct Doc lookupDoc(Pos & pos);

This comment has been minimized.

@edolstra

edolstra Jan 10, 2018

Member

Should be const Pos & pos.

#include "comment.hh"
#include "util.hh"

// This module looks for documentation comments in the source code.

This comment has been minimized.

@edolstra

edolstra Jan 10, 2018

Member

We don't use // in the Nix code base except for single-line comments.

@roberth roberth force-pushed the roberth:lambda-docstring branch from d05552b to c9f618d Jan 11, 2018

@roberth

This comment has been minimized.

Copy link
Author

roberth commented Jan 11, 2018

Before this can be merged we have to make decisions about syntax, because this code will return FIXME comments and such.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.