-
-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Improve lib/trivial and lib/lists docs #23929
Conversation
@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @zimbatm and @peti to be potential reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for docs and fixing the foldr name
/* The identity function | ||
For when you need a function that does “nothing”. | ||
|
||
Type: id :: a -> a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that a new idiom? I don't remember seeing other functions being described by their types.
If it's a new introduction, can I suggest always putting it right next to the function definition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s a style I copied from lib/attrsets
.
Doing that in a consistent manner will greatly help with generating docs.
Ignores the second argument. | ||
Or: Construct a function that always returns a static value. | ||
|
||
Type: const :: a -> b -> a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to express that both a
are the same value as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s implicit in that, like in id
as well.
Since you only have an a
(on which no operations are defined), you can only return it untouched (because there’s nothing you can do with it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in Haskell, where this style of type signatures comes from. In nix it’s possible of course (with type introspection). But that’s evil. :)
lib/lists.nix
Outdated
@@ -6,8 +6,8 @@ rec { | |||
|
|||
inherit (builtins) head tail length isList elemAt concatLists filter elem genList; | |||
|
|||
/* Create a list consisting of a single element. `singleton x' is | |||
sometimes more convenient with respect to indentation than `[x]' | |||
/* Create a list consisting of a single element. `singleton x` is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember @edolstra insisting on that notation for quoting. It's not supposed to be markdown I think (applies also to the other changes in this commit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t see any pro in that notation, but maybe it has an advantage I overlooked. Syntax is the least important aspect of nearly anything, that’s probably why there’s so much bikeshedding. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't matter why are you changing it ? :)
I would also prefer the Markdown notation but it needs to be consistently applied, or at least have some sort of consensus that this is the notation that we want to use. Most of the lib/*.nix files are using that weird quoting syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
related: NixOS/nix#1140 who got reverted later
lib/lists.nix
Outdated
x_1) x_2) ... x_n)'. | ||
/* fold is an alias of foldr for historic reasons */ | ||
# FIXME(Profpatsch): deprecate? | ||
fold = foldr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I don't know the difference and implications between foldl and foldr, which one should I use by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be clarified in the docs, yes. It’s a non-trivial question that depends on the evaluation characteristics of the runtime.
If the operator is not associative, foldl
and foldr
will have different results.
Normally it is, though, so it becomes a question of how many thunks each function uses. There’s an article on HaskellWiki, but I’m not sure if the same rules hold for our nixpkgs folds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I expect you meant to write commutative not associative?
I think the current docs explain which to prefer quite well: use (strict) foldl if you intend to consume the entire result right away, esp. when reducing to atomic values, otherwise you probably want to use foldr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean associative. Commutativity is not needed/used by fold[l/r]
.
cf. the fold
function in Data.Foldable
: it needs a Monoid
, which is associative and has a one-element (for the start value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But commutativity is what matters wrt whether foldr and foldl return the same result, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope:
((10 − 5) − 1) = 4
(10 − (5 − 1)) = 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see what you mean. Carry on :)
I have improved the tests for different fold attributes (e.g. for non-associative functions right folds are not the same as left folds). |
In order to better distinguish foldr from foldl the default name is changed to foldr, but fold is still a synonym. Additionally the docs are improved and examples added.
Also the invocation of tests is documented.
2038a1a
to
4f1d977
Compare
Okay, removed the vanity changes and changed everything over to the strange code quoting syntax @zimbatm. |
Three changes/improvements
fold
foldr
by default, aliasfold
tofoldr
For more information see the commit descriptions.
nix-build lib/tests.nix
exits successfully.