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

doc: primops: add more info for foldl #9254

Merged
merged 2 commits into from
Nov 23, 2023

Conversation

phip1611
Copy link
Contributor

@phip1611 phip1611 commented Oct 29, 2023

Motivation

Improving documentation of builtins.foldl.

From the existing doc it is not obvious whether the first or the second argument of builtins.foldl' is the accumulator. This is however relevant to know, as for certain scenarios, this might change the behavior.

We can solve this by either adding more info (as I did) or by changing the example from foldl' (x: y: x + y) 0 [1 2 3] to foldl' (accumulator: next: accumulator + next) 0 [1 2 3]. Which do you prefer?

Context

For reference:

image

Priorities

Not very important.

@phip1611
Copy link
Contributor Author

PS: it took me quite some time to figure out where I have to add this improvement. The edit button on the documentation site is misleading.

@infinisil
Copy link
Member

Btw the lib.foldl' docs were also improved recently: https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.lists.foldl-prime

src/libexpr/primops.cc Outdated Show resolved Hide resolved
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

@fricklerhandwerk is the doc guru. Looks good?

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Great catch, thanks a lot.

We can solve this by either adding more info (as I did) or by changing the example from foldl' (x: y: x + y) 0 [1 2 3] to foldl' (accumulator: next: accumulator + next) 0 [1 2 3]. Which do you prefer?

I prefer both. But maybe call it item or element instead, because it's the current one being processed, not the next. Functional programming is still a challenge for a large part of our growing audience, so let's try to keep the potential for confusion low.

Please follow the docs contribution guide, in particular the one sentence per line rule.

The edit button is useless for many pages indeed, because they are generated, as you've found. It's a low-effort stop-gap. I'd appreciate and swiftly review PRs to update the contribution guide with more help to find the right things. Another, possibly slightly more scalable (but possibly counterproductive) option would be to trick the button into working by adding symlinks in the locations it expects to where the code actually is, and possibly massaging edit-url-template appropriately.

Side note to @nixos/nix-team: on top of that we may want to follow @infinisil's lead moving contributor-facing documentation in Nixpkgs into the source tree and out of the manual altogether. That would simplify a few things, such as linking to files in their most recent version. (I know, links to files should not be needed with things like doxygen, but reality is messy.) Also, any released manual will be outdated with respect to the code, and we'd anyway want to get (potential) contributors closer to where the action is - on GitHub.

From the existing doc it is not obvious whether the first or the
second argument is the accumulator. This is however relevant to
know, as for certain scenarios, this might change the behavior.
@phip1611
Copy link
Contributor Author

@fricklerhandwerk I've updated the MR with your suggestions. What do you think about the new version?

Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Looks good. I'd usually be picky about formatting according to the style guide, but this can also be done later. Thanks a lot!

src/libexpr/primops.cc Outdated Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk enabled auto-merge (squash) November 23, 2023 20:28
@infinisil
Copy link
Member

Side note to NixOS/nix-team: on top of that we may want to follow infinisil's lead moving contributor-facing documentation in Nixpkgs into the source tree and out of the manual altogether. That would simplify a few things, such as linking to files in their most recent version. (I know, links to files should not be needed with things like doxygen, but reality is messy.) Also, any released manual will be outdated with respect to the code, and we'd anyway want to get (potential) contributors closer to where the action is - on GitHub.

Ah this is where I saw you mention this! I opened a separate issue for this :) #9381

@fricklerhandwerk fricklerhandwerk merged commit 2ce8c96 into NixOS:master Nov 23, 2023
8 checks passed
@phip1611 phip1611 deleted the foldl-doc branch November 23, 2023 22:50
@phip1611 phip1611 mentioned this pull request Nov 24, 2023
@fricklerhandwerk fricklerhandwerk added language The Nix expression language; parser, interpreter, primops, evaluation, etc documentation labels Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation language The Nix expression language; parser, interpreter, primops, evaluation, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants