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

extended args@ explanation #576

Merged
merged 3 commits into from May 30, 2016
Merged

Conversation

qknight
Copy link
Member

@qknight qknight commented Jul 6, 2015

i've build the nix documentation and it builds and looks sane.

@qknight
Copy link
Member Author

qknight commented May 27, 2016

quite sad that there is no care and interest in making the documentation better

@qknight qknight closed this May 27, 2016
@copumpkin
Copy link
Member

copumpkin commented May 27, 2016

I wouldn't take this personally. Please reopen it and let's just try to catch @edolstra or @shlevy's attention. This repo really just needs more people who can review and merge stuff. @shlevy is super busy with life right now, and @edolstra is central to just about every Nix process and still has a job and a life and all that, so I expect a huge chunk of things escape his attention unless explicitly called out (and even then, there are finite hours in a day).

Let's fix the specifics, but let's also fix the process.

When I first got commit access to nixpkgs, people warned me not to merge things that I was unsure about. I imagine there are plenty of Nix experts who are still fairly unsure about the Nix codebase, but things like this are safe and uncontroversial and most of us could review and merge it with no input from @edolstra.

@edolstra, can we just add a few more Nix old-timers (@aszlig, @domenkozar, @7c6f434c, @garbas, and perhaps others who volunteer) to this repo so things aren't quite as blocked on you, with the understanding that deep functionality changes to Nix (the implementation or the language) still require your approval, and to ping you when in doubt? Giving people a sense of ownership over the repo could also start spreading the knowledge a bit more and motivate people to get better acquainted with the codebase.

@copumpkin
Copy link
Member

@edolstra more concretely, I'd create a separate team in the organization for each (major) repository, to make it clear who can commit to each of Nix, NixOps, Hydra, and nixpkgs. I see you currently have a team for NixOps, but it's still not entirely clear who has access to commit to Nix, so we don't even know who to highlight besides you and @shlevy (whom we've learned by observing can also commit here). I'm assuming @rbvermaa can commit, but don't know if anyone else can. Some transparency in who to ping would probably go a long way, but extending that list in the longer run seems like the sustainable approach.


Here <varname>args</varname> is bound to the entire argument, which
is further matched against the pattern <literal>{ x, y, z,
... }</literal>.</para></listitem>

... }</literal>. This <literal>@</literal>-pattern makes mainly sense with an
Copy link
Member

Choose a reason for hiding this comment

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

Remove This

@qknight qknight reopened this May 27, 2016
@qknight
Copy link
Member Author

qknight commented May 27, 2016

@domenkozar thanks for your feedback


but can also be written as:

<programlisting> { x, y, z, ... } @ args: z + y + x + args.a</programlisting>

Here <varname>args</varname> is bound to the entire argument, which
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what this means: args is bound to the entire argument, which is further matched against the pattern { ... }

Copy link
Member

Choose a reason for hiding this comment

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

(this snippet was there before, FYI) The idea is that args gets bound to the whole argument passed to the function, and also the "pattern matches" inside of the { ... } match against the same argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see much advantage with having these two different styles. I have mostly seen the first style in nixpkgs. Could we recommend using the first? I think it's nicer to read code that is consistent.

@shlevy
Copy link
Member

shlevy commented May 27, 2016

FYI, I don't follow this repo but if anyone tags me I'll at least take a look and respond.

@shlevy
Copy link
Member

shlevy commented May 27, 2016

Overall looks good to me, agreed with @domenkozar's comments. Ping me when fixed.

@qknight
Copy link
Member Author

qknight commented May 30, 2016

@domenkozar i've taken your two points into account - thanks.

paul and me have covered/documented it fairly well in the tour anyways:

so if this doesn't make it into the nix documentation, i don't think it would be a huge hit.

@domenkozar
Copy link
Member

@qknight way better now. I'd also add that args don't include default values if function argument is not provided.

@domenkozar
Copy link
Member

@qknight raised a wish to merge this as-is and improve later. I think it already provides value so merging.

@domenkozar domenkozar merged commit 5339ae4 into NixOS:master May 30, 2016
zolodev pushed a commit to zolodev/nix that referenced this pull request Jan 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants