CONTRIBUTING.md: shorten for readability

[wolfgangwalther]

My goal is to remove as much unneeded words as possible, making it easier and quicker to read.

Also fixing smaller issues along the way, ofc.

Of course, any suggestions to make it even shorter, for example by removing redundant information, is very welcome. I think we don't need to cover every case 100%, there can be room to deduce other things for the reader themselves. Better to have it a bit shorter and more people actually read it, than the other way around.

Things done

• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other contributing documentation in corresponding paths.

---

Add a 👍 reaction to pull requests you find important.

[pbsds]

I'm generally positive for more compact, direct and active language. Too direct however may be difficult to read to some, depending to how often you're exposed to it.

After doing a narrow-focused "compression" pass we could then "zoom out" and rethink whether all paragraphs or sections are even needed at all in a followup PR.

Stripping trailing / from urls may save a byte, but the resulting url is semantically different (A trailing slash usually implies /index). Stripping the trailing slash usually causes the server to have to redirect the user, increasing latency. Servers that don't rewrite the url and which make use of relative urls may even break.

[wolfgangwalther]

After doing a narrow-focused "compression" pass we could then "zoom out" and rethink whether all paragraphs or sections are even needed at all in a followup PR.

Yes, that would be good. First - I need to be aware of all the sections, so that's why I am trying to read them and shorten them as is.

Stripping trailing / from urls may save a byte, but the resulting url is semantically different (A trailing slash usually implies /index). Stripping the trailing slash usually causes the server to have to redirect the user, increasing latency. Servers that don't rewrite the url and which make use of relative urls may even break.

I had not considered redirects. Will revert those. Not a readability thing anyway, just my inner nerd...

[wolfgangwalther]

I worked through 30-60% of the file and added it as a fixup to ease picking up on review. It feels like I didn't shorten as much anymore and moved more to fixing mistakes and making things easier to read.

There sure is quite some outdated information in there, but I didn't want to make any substantial changes here.

[pbsds]

This bit is not really connected to the rest of the text? (maybe out of scope for this PR to fix however)

[wolfgangwalther]

Yeah, I stumbled on this as well, but wasn't immediately sure what to do with it. Left it in for now, but this is certainly one of the pieces that needs to be looked at, whether it should just go or whether there is a better place for it.

[pbsds]

Should this maybe be rewritten from third person to first person? People don't really go around remembering guidelines that only "others" have to follow.

[wolfgangwalther]

Once I rewrite this to first person... it becomes immediately clear that this is entirely the wrong place for this, too. This is under the "how to merge PRs yourself" header...

This should probably be in the maintainers section instead.

I will rephrase it here and then we can move it later.

[MattSturgeon]

I will rephrase it here and then we can move it later.

I'm impressed you're able to keep track of so many TODOs; you must have some well organised notes!

[wolfgangwalther]

Nothing special about it.. I just have a veery long list of "starred" notification emails :D

[wolfgangwalther]

Thanks for your feedback, so far! Addressed most of your comments and also went through the document further and added another fixup. Not finished, yet, there are still a few paragraphs left.. but I noticed I was just reading through the last two and nodding along, not making any changes anymore. Will do the remainder in the next iteration with fresh energy :D

[MattSturgeon]

Excellent work going through this very dense document!

I'm excited to see the wording slowly improve over time.

[MattSturgeon]

"it often requires" could be clearer. How about

doing so often requires

Also "digging code" feels grammatically awkward. Usually it'd be "digging into code", however if the goal is to be less verbose we could just say "reading code".

How about replacing "found out" by "understood"? I think that conveys the intent.

While this can often be understood by reading code, PR discussion, or upstream changes, doing so often requires lots of work.

This still feels a bit awkward though. The idea we're trying to convey is that a few minutes spent writing a helpful commit message can save someone several minutes (or worse) investigating.

[wolfgangwalther]

While this can often be understood by reading code, PR discussion, or upstream changes, doing so often requires lots of work.

Went with that for now.

[MattSturgeon]

Perhaps this makes sense as two sentences?

Pull requests should not be squash-merged, as this discards information including detail from commit messages, GPG signatures, and authorship.
Many pull requests don't make sense as a single commit anyway.

I'm not happy with my wording, but I think it highlights that the original sentence could be structured better.

---

Perhaps this is highlighting that we should consider disallowing squash-merges altogether? In most cases a PR author can fix their history themselves. In other cases a committer can usually push to the PR branch.

It is still possible that "allow maintainers to commit" is disabled and the PR author is incapable of rewriting git history... But it seems unlikely, and I'm still not convinced squash-merege is appropriate even in the majority of those cases.

[wolfgangwalther]

Perhaps this is highlighting that we should consider disallowing squash-merges altogether?

I have squash merged a few times. I do it when there is no GPG signature anyway, the PR possibly has some open nits that can be committed from suggestions or there had been fixups (but there would be a single commit in the end anyway) and the author had not been responsive. In this case, it seems simpler to do that, than the alternatives. I always take care to keep a commit message if there was one and also add Co-authored-by: original author.

But yes... I could probably also do this by pushing to the PR's branch... if I could finally figure out how to do that easily without adding every contributor's fork as a remote (not sure whether that is possible, but it's the annoying part for me).

(this might make for a good how-to for committers...)

---

We should probably discuss the merge options in the org-repo?

[wolfgangwalther]

Created NixOS/org#135

[wolfgangwalther]

Pushed an intermediate state of my next fixup, but will continue before the next review iteration down to the end (still a few paragraphs missing). Review optional at this stage.

[wolfgangwalther]

OK, I went through the last parts, the fixup is ready for another review (thanks a lot already, guys!). The whole section "Practical contributing advice" is... so much wishy-washy, it's really hard to tell what is important and what is not. Much of that is just 100 different ways of saying "please, be a nice".

[MattSturgeon]

Looking good!

Not sure if you've fully gone through the last round of review's comments yet, but I see a few threads that are still unaddressed.

[MattSturgeon]

This last bullet point feels very vague. I'm not convinced "artifacts" is a useful term here to communicate what it's trying to say.

I'm not even sure whether it's saying "packages built" or "the final code, after applying the PR's patches", or something else...

Not really an issue with your change, just that the existing wording is poor.

I believe the three bullet points are trying to say:

1. intention/motivation/justification necessity/desirability
2. patches/diff quality
3. final/merged quality

But I'm not 100% sure.

[wolfgangwalther]

For a package, the artifact would be the result of the build. Does the build even work? Do the produced binaries work? etc.

For a NixOS module, this is different.

For a new function in lib - entirely different, again.

I think this is what is meant with "artifacts". Aka the "result of what the code does".

For me, the distinction is not between patch/diff and final/merged, but more along the lines of what our lint + eval workflows do (this would be code quality) and what ofborg builds, running automated and manual tests etc. do (that's artifact stuff).

[wolfgangwalther]

re artifacts: Maybe we can dodge the issue in the generic contribution guidelines by saying something like "3rd step to assess is specific to each type of component and their specific contribution guidelines". We link to those multiple times already.

[MattSturgeon]

For a package, the artifact would be the result of the build. Does the build even work? Do the produced binaries work? etc.

For a NixOS module, this is different.

For a new function in lib - entirely different, again.

I think this is what is meant with "artifacts". Aka the "result of what the code does".

That makes sense. But I guess my confusion highlights that the terminology may not be clear to new contributors.

Not really an issue with your PR, but with the existing wording.

[MattSturgeon]

Maybe we can say something like "does it work" instead of talking about the artifact?

To me, artifact makes sense in the context of a derivation that outputs something, but doesn't really make sense for the other examples. Especially lib functions, which IMO are not really artifacts and don't really produce artifacts.

However all examples have some form of functionality; building/running, module evaluates and produces expected config, lib function evaluates and has passing unit tests, etc.

Perhaps we could iterate on this to get something reasonable:

3. Whether it works and (if relevant) produces working executables

3. Whether it functions and (if relevant) produces functioning output

3. It works and (if relevant) produces working build artifacts

3. Whether it functions correctly and (if relevant) produces functioning packages

EDIT: or replace "working"/"functioning" in my examples here with simply "good" as per your current wording

?

Again, not really blocking this PR.

[MattSturgeon]

Second time seeing "artifacts" used. Again, maybe this is a "me" issue, but I'm not clear what it means in this context.

Looking at the full file, "artifact" is present 3 times, none of which provide any definition or clarity. So I think we can treat this as jargon and either rephrase or define.

In this instance, it feels like it's referring back to the 3-point bullet list (despite that being in a different "section").

[MattSturgeon]

This is the third and final usage of "artifact" in the document.

[wolfgangwalther]

but I see a few threads that are still unaddressed.

Uh, they were hidden under "load more comments", missed them entirely. Will do them in the next round.

[wolfgangwalther]

I applied all specific feedback and squash the commits. Double checked for hidden comments, too ;)

Some of the threads I left open. Might need more discussion, maybe we need to go back to them in a later PR to do "bigger" changes / restructuring etc.

It might not be perfect at this stage, but I think it's already a big improvement in general. And I learned a lot!

Edit: And now pushed as well 🙈

[MattSturgeon]

Skim read the latest changes, as I'm confident previous discussions have been resolved appropriately or left open for future PRs (as appropriate).

Therefore, this LGTM!

Thanks again for doing this work.

[wolfgangwalther]

All the currently open threads, I would like to defer to later PRs.

@pbsds would you like to have another look before merge?

[pbsds]

The changes to # Practical contributing advice greatly change how it reads. It is to my understanding that it was intentionally written in a more informal style to be inviting and approachable. I think maybe @Atemu might be interested in having a look at the changes in that section to ensure the original intent is preserved

[wolfgangwalther]

Since there is other work going on the CONTRIBUTING file, for example see #422793, I'd like to merge this rather soon to avoid dealing with merge conflicts. We can still incorporate feedback from @Atemu afterwards. Any objections, @pbsds?

[pbsds]

Due to a headache I've now mainly checked how the the changes to the # Practical contributing advice section reads on a small scale but I haven't internalized how the changes read in the big picture. I choose however to trust you in this regard, and fixups can be made later. Aside my suggestions below this LGTM 👍

w.r.t the use of "artifact": Prefixing it to "built artifacts" is a decent demystification when packages are concerned, but it applies less to nixos modules. With lib and nixos also in mind, how about "provided functionality or built artifacts"?

[pbsds]

When reading this I feel the same dislike for it as I did back when it was originally written and merged. It ping-pongs a lot back-and forth and largely maintains the stance of "nits are good actually" with an appeal to some nebulous authority with language like "unwritten rules". (I am very much in the camp against nits however, hence my dislike of it.)

I just had to air my thoughts, and I consider substantially changing it in this PR out of scope. 👍

[wolfgangwalther]

I'll leave this thread open for a while, so your thoughts are not immediately forgotten ;)

(this PR feels kind of bottom-up in this regard: I handle almost every nit, but any more substantial change is deferred to later :D)

[wolfgangwalther]

Thanks again to both of you!