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

feat: Introduce digging behaviour #98

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ein-shved
Copy link

I am not sure, do you need this functionality within mainstream. So I made this WIP PR to ask you, should I make it more clear and add tests to be merged?

This may be needed when the project has not such flat structure, as nixpkgs. When different libraries produces their outputs not at toplevel, but dipper.

You may specify common templates to dig into in command arguments. Eg --dig submodule, then this will generate documentation for files like:

 {
   submodule = {...}: {
     /* Doc for foo */
     foo = 1;
     /* Doc for bar */
     bar = 2;
   }
 }

You may force documentation to dig into attrset withing its comment to with DocDig! directive:

 {
   /* DocDig! */
   myDeepLib = {...}: {
     /* Doc for foo */
     foo = 1;
     /* Doc for bar */
     bar = 2;
   }
 }

This may be needed when the project has not such flat structure, as
nixpkgs. When different libraries produces their outputs not at
toplevel, but dipper.

You may specify common templates to dig into in command arguments. Eg
`--dig submodule`, then this will generate documentation for files like:

 {
   submodule = {...}: {
     /* Doc for foo */
     foo = 1;
     /* Doc for bar */
     bar = 2;
   }
 }

You may force documentation to dig into attrset withing its comment to
with `DocDig!` directive:

 {
   /* DocDig! */
   myDeepLib = {...}: {
     /* Doc for foo */
     foo = 1;
     /* Doc for bar */
     bar = 2;
   }
 }

Change-Id: I1518323d8e09a087a7b7d02451437792f5bee98f
@infinisil
Copy link
Collaborator

This doesn't seem very sound, because it would just ignore the function argument which could have important semantics. In particular, right now the examples you gave just generate this:

# test {#sec-functions-library-test}

## `lib.test.foo` {#function-library-lib.test.foo}

Doc for foo

## `lib.test.bar` {#function-library-lib.test.bar}

Doc for bar

With no mention of myDeepLib. And even lib.test.myDeepLib.{foo,bar} would not be right, because that looks like a direct attribute, when it's really nested in a function.

I think for an example like this:

{
  /*
    Add or subtract two numbers.
  */
  addSub = { a, b }: {
    /* Doc for add */
    add = a + b;
    /* Doc for sub */
    sub = a - b;
  };
}

generating something like this would make more sense:

# test {#sec-functions-library-test}

## `lib.test.addSub` {#function-library-lib.test.addSub}

**Type**: `Attrs -> Attrs`

Add or subtract two numbers.

**Argument**
structured function argument

: `a`

  : Function argument

  `b`

  : Function argument

**Returns**

`add`

: Doc for add

`sub`

: Doc for sub

This doesn't work very well if you have more complicated nested doc comments though, so maybe both **Returns** and **Argument** should also be headings.

Ping @hsjobeki as this is relevant for NixOS/rfcs#145 and #91

@ein-shved
Copy link
Author

@infinisil, I agree with you, but only when we are talking about automatic collecting of deeper fields. My PR is about different behaviour. The most close example is flake:

{
  outputs = { self, nixpkgs }: {
    /* Doc for abc */
    result1 = "abc";
    /* Just nixpkgs */
    result2 = nixpkgs;
  };
}

My main goal is to generate doc like next:

# my-flake {#sec-functions-library-flake}

## `lib.my-flake.result1` {#function-library-lib.flake.result1}

Doc for abc

## `lib.my-flake.result2` {#function-library-lib.flake.result2}

Just nixpkgs

For now nixdoc could generate documentation only for lib.my-flake.outputs, which is not very useful in such situations. We do not really want to mention the outputs part - it is obvious. We are interested in nested fields as we could access them right as above documentation said my-flake.result2 from another flake.

I think, it is better to drop the /* DocDig! */ part from PR to left the --dig option and implement automatic documentation collection for function nested fields (as you said in the comment) later.

@hsjobeki
Copy link
Collaborator

hsjobeki commented Jan 9, 2024

Regarding the 'DocDig!' directive, I would say that your implementation is rather limited. It only supports direct attributes and direct let...in expressions.

Any expression other than let ... in and attrset will break it.
Imagine:

  • import
  • assert
  • if ... then ... else
  • partial application
  • lists
  • ...
{
  /* DocDig! */
  deep = a: b: c: if true then {
    /* Nested is 1*/
    nested = 1;
  } else {
    /* Nested is 2*/
    nested = 2;
  };
}

I think a complete implementation of this would collect all doc-comments recursively until the end of the parent expression.
It should also track the path of more nested attributes. But that is super complex, and I suspect implementing it comes close to reimplementing a real nix evaluator.

In comparison, I'll explain how https://noogle.dev works with this problem.

Imagine the following 2 files:

init.nix

 {
   /** 
     This function is used to build our 'lib'
   */
   myDeepLib = {...}: {
     /**
       Docs for foo
     */
     foo = x: x;
      /**
       Docs for bar
     */
     bar = x: x;
   }
 }

lib.nix

{
  lib = myDeepLib {...};
}

We can the observe:

nix-repl > lib
{ foo = <lambda@init.nix>; bar = <lambda@init.nix> }

From that position, we can statically analyze the init.nix file at the given position to retrieve the documentation.

This currently works for attributes and lambdas.
It gets a little more complex with partially applied functions.

Unfortunately, this doesn't work with nixdoc because it is a static (ast) only tool. And the nix language is too complex for complete static analysis.

@infinisil
Copy link
Collaborator

The most close example is flake:

{
  outputs = { self, nixpkgs }: {
    /* Doc for abc */
    result1 = "abc";
    /* Just nixpkgs */
    result2 = nixpkgs;
  };
}

You can do this instead:

{
  outputs = inputs: import ./outputs.nix inputs;
  # This doesn't work because of https://github.com/NixOS/nix/issues/4945
  # outputs = import ./outputs.nix;
}

Then you can run nixdoc on ./outputs.nix

@ein-shved
Copy link
Author

You can do this instead:

{
  outputs = inputs: import ./outputs.nix inputs;
  # This doesn't work because of https://github.com/NixOS/nix/issues/4945
  # outputs = import ./outputs.nix;
}

Then you can run nixdoc on ./outputs.nix

Breaking the code architecture to fix documentation tool drawbacks is a very bad idea

@hsjobeki
Copy link
Collaborator

Personally i'd not want to maintain the docDig! directive. I am afraid that this may end up in nixpkgs. Opening up directives that do special "Walk" operations on the AST is not something that i imagined when writing this rfc NixOS/rfcs#145.

Maybe we can find a solution that solves your problem in another way. Do you have some code example / repository that you could show to us?

@ein-shved
Copy link
Author

ein-shved commented Jan 16, 2024

@hsjobeki I do not actually need for DocDig! directive. I've added it as PoC and about to remove it from this PR (I said that previously). Without this option will the functionality of this PR suites this tool?

P.S. The --dig command-line option will left...

@infinisil
Copy link
Collaborator

Rethinking this a bit, I think this could be made to work like this:

{
  outputs =
    { self, nixpkgs }:
    /* nixdoc!outputs */
    {
      /* Doc for abc */
      result1 = "abc";
      /* Just nixpkgs */
      result2 = nixpkgs;
    };
}

Here, nixdoc would search specifically for /* nixdoc!<prefix> in the AST, and render documentation for the found node under the given <prefix>.

This fixes the problem brought up by @hsjobeki in #98 (comment).

@hsjobeki hsjobeki mentioned this pull request Feb 25, 2024
6 tasks
@infinisil infinisil marked this pull request as draft April 9, 2024 18:04
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.

3 participants