Linter/formatter should enforce coding style rules that will gate merging PRs

[pcowgill]

See this issue ipfs-inactive/js-ipfs-http-client#1225

Thanks!

[pcowgill]

@hugomrdias @daviddias @victorb @achingbrain Think this is a good idea? Thanks!

[achingbrain]

I’m not sure that the problem is with the linter. That is, it would be hard to automate the generation a warning for reformatting of code that has little to do with the intended change behind a PR, since the linter has no idea what the intention behind an edit is.

As a rule a PR should be small and focussed and change as little as possible to make the change required.

Doing code reviews is really hard because you only see the code, not the thought behind the code, so anything you can do to lower the cognitive load on the reviewer is incredibly helpful.

Smaller change sets are thus easier to review, easier to navigate via git history, etc.

Fundamentally it’s down to the author to consider if their changes are relevant to fixing the thing they are trying to fix before submitting them.

I know I personally do a final sweep of any PR and remove extraneous edits before pushing them. Ok, sometimes right after pushing them.

[pcowgill]

@achingbrain Hmm, perhaps my meaning wasn't clear. It's straightforward enough to modify the linter and/or formatter config to throw errors if there's space before the parens in a function declaration, etc.

[pcowgill]

Such a setting would lower the cognitive load on PR creators, which would be great!

[pcowgill]

I already usually follow all of those norms you mentioned for the reasons you mentioned, and I apologize for missing that in the PR linked above - I'm saying it would be nice to automate as many of those norms for the dev as possible with linter and/or formatter settings.

[hugomrdias]

This is exactly what our eslint config does, closing this issue.
Feel free to re-open if you think you have more to add.

[pcowgill]

I think we should reopen it. I’m pretty sure I ran the linter and it didn’t catch the issue linked to above. I can try to reproduce it to confirm.

[pcowgill]

@hugomrdias Just confirmed that - making a change such as this:

if (options.enableShardingExperiment != null) searchParams.set('enable-sharding-experiment', options.enableShardingExperiment)

->

if (options.enableShardingExperiment != null) {
  searchParams.set('enable-sharding-experiment', options.enableShardingExperiment)
}

doesn't create any new warnings or errors. So I think this issue should be reopened. Here are the issues like this I noticed (including the one referenced above):

ipfs-inactive/js-ipfs-http-client#1224 (comment)
ipfs-inactive/js-ipfs-http-client#1224 (comment)
https://github.com/ipfs/js-ipfs-http-client/pull/1224/files#r369679391

[hugomrdias]

that happens when you have extra formatters in your editor or manual change formatting. we use eslint-standard and while it doesn't have every single style rule defined to avoid these issues it is enough for our needs.
we might adopt prettier in future but for now it is what it is.
In general when contributing one should try not to add changes to the formatting to make PRs easier to review, we haven’t had any major problems with this so priority is pretty low.

[pcowgill]

Yep, I know. I thought this might increase the odds of helpful PRs making it in going forward, but that's a very reasonable prioritization choice. Thanks!