editorconfig enforcement for PR is counterproductive

[domenkozar]

Continuing discussion from #120959

I think this lacks empathy to contributors

Editors support .editorconfig via plugins and some will strip whitespace/newlines/etc unless disabled. I don't see the need for much empathy really, it's trivially handled once in the editor and forgotten about.

Can we merge this PR to deal with the immediate problem and do the editorconfig good/bad discussion elsewhere?

I'd like to ask in that case, why enforce it then? If it's automatically handled, there's no need to enforce it. If it's not automatically handled, then we're wasting time from people wanting to contribute.

[zowoq]

I'd like to ask in that case, why enforce it then?

Because we can't enforce it client side.

[domenkozar]

Now we're back to my original point for which you provided an argument that it's automatic anyway so there's no need for empathy?

[zowoq]

Yes?

We expect people to format commit messages a certain way, we expect ofborg to pass eval, we (usually) expect builds to pass.

Why is not introducing whitespace, trailing newlines and tabs such an inconvenience?

With the action it is at least consistent, we have rules and they are followed.

Without the action it's a free for all, sporadically enforced by reviewers and contributors.

Is that a better experience for contributors, having some people asking for fixes and others ignoring it?

[domenkozar]

We expect people to format commit messages a certain way, we expect ofborg to pass eval, we (usually) expect builds to pass.

Why is not introducing whitespace, trailing newlines and tabs such an inconvenience?

Good question. We already specify too many rules, so adding extra ones means we have to be conscious that each rule adds cognitive and work overhead to each contribution.

Having extra whitespace has absolutely no downsides development-wise. If it disturbs people where the editor highlights trailing whitespace, disable that and you're done.

Is that a better experience for contributors, having some people asking for fixes and others ignoring it?

Reviewers shouldn't nitpick.

[zowoq]

https://github.com/NixOS/nixpkgs/pull/86376/files

This is the PR that caused the editorconfig check, it removed final newlines from ~200 files. Is that a nitpick?

[avnik]

Odd side -- extra whitespaces and trailing newline add/removal create unnessesary diffs

[domenkozar]

https://github.com/NixOS/nixpkgs/pull/86376/files

This is the PR that caused the editorconfig check, it removed final newlines from ~200 files. Is that a nitpick?

It's not, because it didn't waste 200 contributors' time and it was done once.

[domenkozar]

I see the following options to resolve the current situation:

• autocommit the changes (not sure we can do that securely?)
• explain to the user when CI check fails what they should do
• remove the check all together

[zowoq]

Just to have some numbers:

The check has run 44,175 times.

It's errored (requiring further action) 1500 times.

[zowoq]

This is the PR that caused the editorconfig check, it removed final newlines from ~200 files. Is that a nitpick?

It's not, because it didn't waste 200 contributors' time and it was done once.

If it's not a nitpick how do we prevent similar things from being merged again?

Also those ~200 files we then edited by other people, some of who readded the removed newlines (IIRC ~50 or so of them at the time we added the check) so it isn't just something that happens "once", it's ongoing noise being added to the git history.

[alyssais]

explain to the user when CI check fails what they should do

I like the sound of this one

[grahamc]

Agreed, @alyssais. I think the editorconfig is really useful for getting everybody's editors essentially configured the same in a lot of fundamental ways. I find this improves collaboration and reduces nitpicks or weirdness like editing a 4-space indented file from someone who didn't know and having your editor start adding 2 space indentations. Editorconfig is a fairly widely adopted standard, GitHub's web editor respects it, and essentially every text editor of significant use has built-in support or plugin.

In summary I'm a big fan of educating users on how to configure their editor for editorconfig. It'll improve their experience when contributing to a wide variety of other OSS projects too.

[domenkozar]

My problem is that we jumped into enforcing it, without educating the users.

IOW, I am not against using editorconfig, but only when things are enforced prematurely.

I've opened #121063 that improves that, but I don't think it's where it should be, but I don't have much time to spend on this further. I still think enforcement does more harm than it brings the benefits, but I hope I've shown my willingness to improve that.

[dotlambda]

Maybe we can add a paragraph to https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md suggesting people enable editorconfig support in their editors?
Alternatively, it might be part of the PR template.

[FRidh]

Is the editorconfig job still necessary if nixpkgs-fmt is used? #120832 #121508

[Synthetica9]

Is the editorconfig job still necessary if nixpkgs-fmt is used? #120832 #121508

Yes, since nixpkgs-fmt only applies to .nix files. Nixpkgs isn't just .nix files, but also a whole bunch of scripts and other bits and bobs

(maybe we could disable the editorconfig job for .nix files specifically, I don't see the advantage of that tho)

[Synthetica9]

We could also provide a suggested pre-commit-hook for nixpkgs. This might honestly be the best way to approach this:

• We can run this one-on-one in the github actions check that currently checks editorconfig
• We can add more things to this down the line (think nixpkgs-fmt, but also if we decide all python files should be formatted with black in two years)
• We have an easy way to explain "do this and the check won't trigger" no matter what editor people are using.

[domenkozar]

We could also provide a suggested pre-commit-hook for nixpkgs. This might honestly be the best way to approach this:

• We can run this one-on-one in the github actions check that currently checks editorconfig
• We can add more things to this down the line (think nixpkgs-fmt, but also if we decide all python files should be formatted with black in two years)
• We have an easy way to explain "do this and the check won't trigger" no matter what editor people are using.

I agree, but I'd like to suggest reducing the scope to get us started. We can work on improving the developer experience once we have the formatting in place.

[Artturin]

editorconfig enforcement is not counterproductive