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

lib: Add contribution guidelines #272083

Merged
merged 1 commit into from
Dec 19, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions lib/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,69 @@ The [module system](https://nixos.org/manual/nixpkgs/#module-system) spans multi
- [`options.nix`](options.nix): `lib.options` for anything relating to option definitions
- [`types.nix`](types.nix): `lib.types` for module system types

## PR Guidelines

Follow these guidelines for proposing a change to the interface of `lib`.

### Provide a Motivation

Clearly describe why the change is necessary and its use cases.

Make sure that the change benefits the user more than the added mental effort of looking it up and keeping track of its definition.
If the same can reasonably be done with the existing interface,
consider just updating the documentation with more examples and links.
This is also known as the [Fairbairn Threshold](https://wiki.haskell.org/Fairbairn_threshold).

Through this principle we avoid the human cost of duplicated functionality in an overly large library.

### Make one PR for each change

Don't have multiple changes in one PR, instead split it up into multiple ones.

This keeps the conversation focused and has a higher chance of getting merged.

### Name the interface appropriately
Copy link
Member

Choose a reason for hiding this comment

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

This section feels a bit arbitrary. I agree that naming is important, but I'm having a hard time to figure out if a name is good or bad, only based on this information.

Providing examples of good and bad names accompanied with explanations, or links to similar functional libraries (maybe hoogle or elm?) might be a good addition. Or maybe recommend people to read through earlier PRs tagged with https://github.com/NixOS/nixpkgs/labels/6.topic:%20lib, to see the name discussions there and get a feeling for it?

Copy link
Member

Choose a reason for hiding this comment

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

If we're going to refer to languages, we'd have to point out the differences as well. That is, link to a document that does that.
Here's a few things:

  • Nix lists don't have lazy spines like in Haskell, nor is cons cheap.
  • Maybe and Nullable are very different. mapNullable is not a lawful functor. It behaves like >>= / flatMap. Nullable (Nullable a) == Nullable a, etc. Less suitable for composition.
  • Haskell Map.union is flipped compared to //.

Copy link
Member

Choose a reason for hiding this comment

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

Until we codify anything: maybe make it "here's what we think" and "if you don't know, just open a PR and let's talk about it"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added

If you don't know what name to choose, indicate this as a discussion point in the PR.

to this item. I don't think we could have very concrete guidelines here for now.


When introducing new names to the interface, such as new function, or new function attributes,
make sure to name it appropriately.

Names should be self-explanatory and consistent with the rest of `lib`.
If there's no obvious best name, include the alternatives you considered.

### Write documentation

Update the [reference documentation](#reference-documentation) to reflect the change.

Be generous with links to related functionality.

### Write tests

Add good test coverage for the change, including:

- Tests for edge cases, such as empty values or lists.
- Tests for tricky inputs, such as a string with string context or a path that doesn't exist.
- Test all code paths, such as `if-then-else` branches and returned attributes.
- If the tests for the sub-library are written in bash,
test messages of custom errors, such as `throw` or `abortMsg`,

At the time this is only not necessary for sub-libraries tested with [`tests/misc.nix`](./tests/misc.nix).

See [running tests](#running-tests) for more details on the test suites.

### Write tidy code

Name variables well, even if they're internal.
The code should be as self-explanatory as possible.
Be generous with code comments when appropriate.

As a baseline, follow the [Nixpkgs code conventions](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#code-conventions).

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
As a baseline, follow the [Nixpkgs code conventions](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#code-conventions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Not a fan of that section, but sure we have it, let's reference it. Most of it should become obsolete with NixOS/rfcs#166 :)

### Write efficient code

Nix generally does not have free abstractions.
Be aware that seemingly straightforward changes can cause more allocations and a decrease in performance.
That said, don't optimise prematurely, especially in new code.

## Reference documentation

Reference documentation for library functions is written above each function as a multi-line comment.
Expand Down