-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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: lib/textClosures #351872
Open
hsjobeki
wants to merge
1
commit into
NixOS:master
Choose a base branch
from
hsjobeki:textClosures
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
doc: lib/textClosures #351872
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
To me, this seems to be what the function is doing. It is used this way in
php.buildEnv
in order to sort import statements by the order they depend on each other. However I'm not the maintainer or a direct user of the function, so please take it with a grain of salt.Is it okay to use the AttrsOf notation? Also, I'm not sure what was meant with the "almost but not quite computes a topological sort" comment. I wonder if there are some edge cases left over or if it's just that it doesn't fill the formal definition of a topological sort somehow.
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 also found
textClosureMap
to be used in the activation. Those list entries are generated fromnoDepEntry
fullDepEntry
orpackEntry
as the header suggests. But their meaning is not explained either.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.
Looking at them, they just seem like helpers, similar to how
lib.nameValuePair
is meant to be used withlib.listToAttrs
.textClosureMap
is probably tolib.textClosureList
likelib.concatMapStringsSep
is tolib.concatStringsSep
. Not exactly, but in the sense that it's purpose is to be a shortand to map something before thetextClosureList
operation (and in this case concat with newlines as well).We could reuse the helpers in the example to encourage their usage. I'm not sure if they are actually that useful though, might only be unnecessary overhead. What do you think about this?
As for
packEntry
, it doesn't seem to be used anywhere. I can't think of a good usecase for anonymous nodes at the moment ¯\_(ツ)_/¯stringAfter
looks like it's used around the module system. Usage looks similar to how it's used in the home-manager DAG system, used for specifying that something should come after something else in a resulting string/file, overriding the order the module system might merge it in. See also doc and usage example.Considering the use in the module system, it might make more sense to write about the helpers not in the context of usage with the closure functions directly, but rather together with the module system. Maybe not in their doc comments, but in the nixos manual somewhere? What do you think?