lib: Add contribution guidelines #272083

infinisil · 2023-12-04T16:52:02Z

Description of changes

This should be written down so we can refer to it.

This work is sponsored by Antithesis ✨

Add a 👍 reaction to pull requests you find important.

roberth

Unfortunately we can't automate ourselves away, but better expectations and structure are great goals.

I just hope people will find it. Maybe link it from relevant other contributing oriented pages? I'm thinking the general coding guidelines, and maybe the PR template?

roberth · 2023-12-04T19:18:50Z

lib/README.md

+## Contributing
+
+Follow these guidelines for proposing a change to the interface of `lib`:
+- Motivation: Clearly describe why the change is necessary and its use cases


Suggested change

- Motivation: Clearly describe why the change is necessary and its use cases

- Provide a motivation: Clearly describe why the change is necessary and its use cases

If your suggestion is to use imperative titles, I believe this is better:

- Do that thing <More details about why doing that things is good>

roberth · 2023-12-04T19:20:29Z

lib/README.md

+    If the same can 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).
+- Single PR: Don't have multiple changes in one PR, instead split it up


Suggested change

- Single PR: Don't have multiple changes in one PR, instead split it up

- Single purpose PR: Don't have multiple changes in one PR, instead split it up. It keeps the conversation focused and has a higher chance of getting merged.

lib/README.md

roberth · 2023-12-04T19:32:23Z

lib/README.md

+  - If there's no obvious best name, include the alternatives you considered
+- Documentation: Update the reference documentation to reflect the change
+  - Add links to related functions
+- Tests: Add reasonable test coverage for the change


What do we consider reasonable? Can we provide a rule of thumb?
Some recommendations I would suggest:

Suggested change

- Tests: Add reasonable test coverage for the change

- Tests: Add good test coverage for the change

- For each parameter, test edge cases of the types such as empty values.

- Look for tricky inputs, to a reasonable degree. For example: a string with a string context (e.g. adding some file to the store), or a path value that points to a non-existing path if that should be accepted.

- Make sure to cover all code paths, except perhaps assertions that relate to internal invariants, as those may be impossible to trigger.

- We don't require checks on error messages for files tested by test suites that don't support this. For new error messages in the module system, we do.

lib/README.md

infinisil · 2023-12-04T23:14:53Z

@roberth I incorporated your feedback, though I did a bit of rewriting for a more consistent style (diff)

roberth

Love the new layout.
Should the items be headers though? That way we get to link to them too.

lib/README.md

roberth · 2023-12-05T15:25:45Z

lib/README.md

+
+  Name variables well, even if they're internal.
+  Add code comments where the code isn't self-explanatory.
+


Suggested change

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

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 :)

lib/README.md

infinisil · 2023-12-05T19:03:25Z

Should the items be headers though? That way we get to link to them too.

I did try that out briefly but decided against it because I don't want people to just link to one part of it. This section should be short enough that people can read the entire one without focusing on just one aspect.

I applied all your suggestions 🙂 (diff)

roberth · 2023-12-05T19:13:05Z

I don't want people to just link to one part of it.

Why's that?

This section should be short

I agree, but seems quite unrelated.

infinisil · 2023-12-05T19:35:45Z

I don't want people to just link to one part of it.

Why's that?

Hmm I didn't think it's needed, but I guess I don't mind very much, I'll change that.

Co-Authored-By: Robert Hensing <[email protected]>

roberth · 2023-12-07T19:22:14Z

Actually would be good to have links. I mentioned it in the review summary/preface thingy, which is too easy to overlook. Bad UI.

Maybe link it from relevant other contributing oriented pages? I'm thinking the general coding guidelines, and maybe the PR template?

infinisil · 2023-12-07T21:21:08Z

Oh yeah, sorry I was a bit sloppy with properly addressing the feedback this time.

I just hope people will find it. Maybe link it from relevant other contributing oriented pages? I'm thinking the general coding guidelines, and maybe the PR template?

I am fairly happy with the overview section in CONTRIBUTING.md, which links to lib/README.md

h7x4 · 2023-12-08T05:37:02Z

lib/README.md

+
+This keeps the conversation focused and has a higher chance of getting merged.
+
+### Name the interface appropriately


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?

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 //.

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"?

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.

roberth · 2023-12-11T09:37:27Z

lib/README.md

+
+Names should be self-explanatory and consistent with the rest of `lib`.
+If there's no obvious best name, include the alternatives you considered.
+If you don't know what name to choose, indicate this as a discussion point in the PR.


"indicate as discussion point" sounds really heavy.

This sentence also conflicts a bit with the preceding one. Do we really think contributors can't come up with any name whatsoever? It actually seems condescending.

Suggested change

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

or

Suggested change

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

If you don't know what name to choose, feel free to say so in the PR description.

I don't really mind either way, ping @h7x4

@h7x4 I was hoping to get some thoughts 😅

Alright I'll just remove this sentence again. Let's merge before this stalls out for too long. Feel free to open follow-up PRs if there's something to improve.

Sorry, the ping went by without me noticing. The thumbs up was intended for the entirety of roberth's comment, so if you removed the sentence I think all is good!

Cool thanks!

infinisil requested a review from roberth December 4, 2023 16:52

github-actions bot added the 6.topic: lib The Nixpkgs function library label Dec 4, 2023

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 4, 2023

roberth reviewed Dec 4, 2023

View reviewed changes

infinisil force-pushed the lib-contrib branch 4 times, most recently from a421656 to 5925f69 Compare December 4, 2023 23:12

infinisil mentioned this pull request Dec 5, 2023

lib.packagesFromDirectoryRecursive: init #270537

Merged

13 tasks

roberth reviewed Dec 5, 2023

View reviewed changes

infinisil force-pushed the lib-contrib branch from 0d5bd8a to 585ebba Compare December 5, 2023 19:02

lib: Add contribution guidelines

395fc06

Co-Authored-By: Robert Hensing <[email protected]>

infinisil force-pushed the lib-contrib branch from 585ebba to 395fc06 Compare December 5, 2023 19:38

roberth approved these changes Dec 7, 2023

View reviewed changes

h7x4 reviewed Dec 8, 2023

View reviewed changes

infinisil force-pushed the lib-contrib branch from 395fc06 to 0476c4d Compare December 10, 2023 23:44

roberth reviewed Dec 11, 2023

View reviewed changes

infinisil force-pushed the lib-contrib branch from 0476c4d to 395fc06 Compare December 19, 2023 21:28

infinisil merged commit 0f8d175 into NixOS:master Dec 19, 2023

infinisil deleted the lib-contrib branch December 19, 2023 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: Add contribution guidelines #272083

lib: Add contribution guidelines #272083

infinisil commented Dec 4, 2023

roberth left a comment

roberth Dec 4, 2023

AndersonTorres Dec 4, 2023

roberth Dec 4, 2023

roberth Dec 4, 2023

infinisil commented Dec 4, 2023

roberth left a comment

roberth Dec 5, 2023

infinisil Dec 5, 2023

infinisil commented Dec 5, 2023 •

edited

Loading

roberth commented Dec 5, 2023

infinisil commented Dec 5, 2023

roberth commented Dec 7, 2023

infinisil commented Dec 7, 2023

h7x4 Dec 8, 2023

roberth Dec 9, 2023

roberth Dec 9, 2023

infinisil Dec 10, 2023

roberth Dec 11, 2023

infinisil Dec 11, 2023

h7x4 Dec 12, 2023

infinisil Dec 13, 2023

infinisil Dec 19, 2023

h7x4 Dec 20, 2023

infinisil Dec 20, 2023

	- Motivation: Clearly describe why the change is necessary and its use cases
	- Provide a motivation: Clearly describe why the change is necessary and its use cases

	- Single PR: Don't have multiple changes in one PR, instead split it up
	- Single purpose PR: Don't have multiple changes in one PR, instead split it up. It keeps the conversation focused and has a higher chance of getting merged.

-- Tests: Add reasonable test coverage for the change
+- Tests: Add good test coverage for the change
+  - For each parameter, test edge cases of the types such as empty values.
+  - Look for tricky inputs, to a reasonable degree. For example: a string with a string context (e.g. adding some file to the store), or a path value that points to a non-existing path if that should be accepted.
+  - Make sure to cover all code paths, except perhaps assertions that relate to internal invariants, as those may be impossible to trigger.
+  - We don't require checks on error messages for files tested by test suites that don't support this. For new error messages in the module system, we do.


		Name variables well, even if they're internal.
		Add code comments where the code isn't self-explanatory.


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


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

		### Name the interface appropriately

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

lib: Add contribution guidelines #272083

lib: Add contribution guidelines #272083

Conversation

infinisil commented Dec 4, 2023

Description of changes

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Dec 4, 2023

roberth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Dec 5, 2023 • edited Loading

roberth commented Dec 5, 2023

infinisil commented Dec 5, 2023

roberth commented Dec 7, 2023

infinisil commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil commented Dec 5, 2023 •

edited

Loading