require unification proposal

[vsemozhetbyt]

"How to write a test..." guide advises require alphabetical sorting for new tests, but our codebase mostly does not follow this rule (some logical or random order is used).

I can try to revisit all require sections in benchmarks, libs, and tests. And while I am at it, all these things also can be addressed:

1. Sort.
2. Up and consolidate all require in one place when it is appropriate (no conditionals involved).
3. Use destructuring.
4. Insert a blank line after require section (not sure if any rules like this or this ones are required, but at least this can be unified).

However, this will be a time-consuming task, so I need to be sure if:

1. This makes any sense and is not chasing my tail.
2. Someone will have time to review.

[addaleax]

Tbh, I find that rule superfluous, and maybe deleting it from the doc is the easiest approach. If our tests required enough modules to not immediately see which line requires which module, maybe, but that’s hardly true.

Also, it could actually be good not to always follow it; require()ing core modules is not always free of side-effects, so testing different orders might not be a bad thing.

[vsemozhetbyt]

FWIW, refs: #10616

[refack]

@sam-github makes a good point:

So maybe amend that rule to apply only when more than N requires are used (let's say N=5)?
Also as a follow up, what do we recommend exactly regarding destructuring?
Suggestion: If more than two functions from the module are used, destructuring is recommended (unless the functions have very generic names).

[Trott]

So maybe amend that rule to apply only when more than N requires are used (let's say N=5)?
...
If more than two functions from the module are used, destructuring is recommended (unless the functions have very generic names).

I prefer that we have fewer and simpler rules. I'd rather leave these things to each individual's judgment than have multiple "if N is greater than 4, do X otherwise do Y" rules.

When the "alphabetize the modules" rule was first suggested, I was neutral. Since then, I've become -0 on it. It seems low value to me, at least in tests. Most tests don't follow the rule. It's one more thing newcomers have to learn. And I generally dislike rules for code that go unflagged by our tooling.

So I'd be OK with just removing the rule, especially if the alternative is low-value churn in hundreds of files.

[refack]

I think the usual rationale behind sorting dependencies is to eliminate duplications. Something we currently don't lint for.
This passes our linter:

const { exec } = require('child_process');
const { spawn } = require('child_process');
const child_process = require('child_process');
const cp = require('child_process');
console.log(child_process);
console.log(cp);

I'll try to find a way to lint for that.

[sam-github]

Just to point out, while its true that most of our current tests don't alphabetize requires, its also true that most don't have a descriptive comment. The point of the test guide wasn't to encode current practice, but to describe what we thought it should be. And one point of the sorting which isn't effected at all by whether there is one or 20 requires is to make modification easy, so when a require is added maintainers don't have to try to deduce the personal preference of the original author: new on the top, new on the bottom, grouped by some "I feel these are similar" criteria, random, ...?

Which is why even though I personally dislike const {exec, spawn} = require('child_process'); because it breaks the "write code that git diffs well" guideline, I will happily code that way in node because it means I don't have to think about it, and be even happier if lint tells me what the guidelines are when I break them.

[vsemozhetbyt]

It seems this is a questionable approach, so I shall close for now. Thank you for the feedback.