Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add lint rule to enforce trailing commas #45471

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 15, 2022

No description provided.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/modules
  • @nodejs/net
  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 15, 2022
@Trott
Copy link
Member

Trott commented Nov 15, 2022

This LGTM, but a possibility to consider: We get nits on the docs by people removing trailing commas from time to time. And that's with only having trailing commas in a few places! It's an atypical coding style.

I'm fine with turning it on, and in the unlikely event that we end up with an avalanche of confused readers, we can turn it off again (in docs only).

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Nov 15, 2022
@ljharb
Copy link
Member

ljharb commented Nov 15, 2022

@Trott fwiw in the wider JS world, i'm pretty confident it's almost universal to enforce trailing commas on any multiline construct, to minimize diff churn

@tniessen
Copy link
Member

I don't know how much the diff churn argument matters for documentation examples. I totally get that those who like trailing commas would want them here and elsewhere for consistency, but looking at documentation of individual functions, for example, I personally don't think it helps readability:

import {
  setTimeout,
} from 'timers/promises';

@Trott
Copy link
Member

Trott commented Nov 15, 2022

@Trott fwiw in the wider JS world, i'm pretty confident it's almost universal to enforce trailing commas on any multiline construct, to minimize diff churn

What makes you say it is "almost universal to enforce trailing commas"?

ESLint's comma-dangle rule is enabled as part of their "recommended" rules. It defaults to prohibiting all dangling commas.

The standard package prohibits all dangling commas.

I could go on, but that should really be enough to make it clear that dangling commas are definitely not "almost universal". On the contrary, it's a pretty strong indicator that dangling commas are atypical.

@Trott
Copy link
Member

Trott commented Nov 15, 2022

MDN uses trailing commas in docs, so that's good enough for me.

image

@ljharb
Copy link
Member

ljharb commented Nov 15, 2022

The airbnb style guide, and prettier i believe, have required them for many many years, which constitute far more usage than the eslint recommended config - but fair counterpoint, and certainly it’s not worth arguing about it :-)

@Trott

This comment was marked as off-topic.

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 17, 2022
@nodejs-github-bot nodejs-github-bot merged commit c6dabe3 into nodejs:main Nov 17, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in c6dabe3

@aduh95 aduh95 deleted the trailing-commas-doc branch November 17, 2022 14:14
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
PR-URL: nodejs#45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45471
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants