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

args: omit all lamda args. To force explizit documentation of them. #109

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

hsjobeki
Copy link
Collaborator

@hsjobeki hsjobeki commented Mar 8, 2024

This fixes a problem, that arose during the migration to doc-comments.

@DanielSidhion and me already started migrating to doc-comments (lib.asserts is already migrated), and encountering having arguments double documented. Just deleting the adjacent comments can't fix this.

With this PR we can assume arguments are documented explicitly in the lambda documentation as:

{

/** 
# Inputs

foo

: some foo docs

*/
f = foo: 1;

}

The issue is, that we automatically create empty default documentation for non documented arguments. (which is fine for the legacy path)

I've found it fine to assume that having a doc-comment is enough to assume that there is no need to magically add argument documentation for now, since the doc-comment should already includes those.

At a later point in time, we may add argument documentation for doc-comments as well. Because we specified them for lambda formals (structured arguments) I am still working on the roadmap how to integrate them.

@hsjobeki
Copy link
Collaborator Author

hsjobeki commented Mar 8, 2024

@infinisil Some context

image

@hsjobeki hsjobeki requested a review from infinisil March 8, 2024 13:23
@hsjobeki hsjobeki force-pushed the feat/lambda-arguments branch 2 times, most recently from 2eb7ef7 to 8bcce0c Compare March 8, 2024 13:34
@hsjobeki hsjobeki force-pushed the feat/lambda-arguments branch from 8bcce0c to 76ad63b Compare March 8, 2024 13:35
@DanielSidhion
Copy link

DanielSidhion commented Mar 8, 2024

To give some more context for the future:

We plan on eventually supporting docstrings on the function AND docstrings on the function arguments at the same time. Doing that requires us to understand the layout of function arguments on the overall function documentation.

If we wanted to, we could've read the current documentation for all functions in lib and figured out a layout that would work for every case. However, the current documentation is very poor, so whatever decision we make right now might not be a good one for the near future.

Instead, we decided to:

  • Adopt the most flexible documentation approach (ignore argument docstrings, trust that the function docstring talks about the arguments in whatever format it wants)
  • Wait until efforts to go through the Nixpkgs manual to update documentation makes a pass at the lib functions.
  • At that point, look at all the ways that function arguments are documented and figure out a layout that would work well for all cases, and then work backwards to define how to combine function docstrings + function argument docstrings

Copy link
Collaborator

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Looking good to me!

That also sounds like a great plan :D

@infinisil infinisil merged commit a187d87 into nix-community:master Mar 12, 2024
2 checks passed
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