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

fix: ignore spacing before ] and } in comma-spacing #16113

Merged
merged 1 commit into from Jul 8, 2022
Merged

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 7, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Fixes #16100

Updates the comma-spacing rule to ignore spacing before ] and spacing before } in order to avoid conflicts with array-bracket-spacing and object-curly-spacing rules.

For example, the current version of comma-spacing reports an error on each of the following lines, but fixing them triggers errors from array-bracket-spacing and object-curly-spacing rules:

/* eslint comma-spacing: "error" */
/* eslint array-bracket-spacing: "error" */
/* eslint object-curly-spacing: "error" */

[a,]; // comma-spacing reports missing space after `,` but fixing it triggers array-bracket-spacing

[a,] = foo; // comma-spacing reports missing space after `,` but fixing it triggers array-bracket-spacing

obj = {a,}; // comma-spacing reports missing space after `,` but fixing it triggers object-curly-spacing

var {a,} = obj; // comma-spacing reports missing space after `,` but fixing it triggers object-curly-spacing

import {b,} from "mod"; // comma-spacing reports missing space after `,` but fixing it triggers object-curly-spacing

Playground link

Note that this rule already ignores spacing after [ (#3392) and spacing before ) (#11295).

What changes did you make? (Give an overview)

  • Merged validateCommaItemSpacing into the loop in "Program:exit"() because the logic of what to check and what to ignore was unnecessarily scattered.
  • Removed JSXText check. We are checking if it's a comma punctuator token just a line before, so it can't be JSXText.
  • Added new checks for ] and }.
  • Updated the docs accordingly.

Is there anything you'd like reviewers to focus on?

I also wanted to replace addNullElementsToIgnoreList and all related code with a simple [ token check, but it turned out that the existing logic has possibly unintentional side effects of ignoring spacing between a comment and ending comma of a hole/elision.

This avoids conflicts with array-bracket-spacing and object-curly-spacing rules.

Fixes #16100
@mdjermanovic mdjermanovic added bug ESLint is working incorrectly rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Jul 7, 2022
@netlify
Copy link

netlify bot commented Jul 7, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 2d81db4
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62c622f5f7b25d0008dbdc7e
😎 Deploy Preview https://deploy-preview-16113--docs-eslint.netlify.app/rules/comma-spacing
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

{
code: "var arr = [1 , ];",
output: "var arr = [1 ,];",
options: [{ before: true, after: false }],
errors: [
{
message: "There should be no space after ','.",
type: "Punctuator"
}
]
},
Copy link
Member Author

@mdjermanovic mdjermanovic Jul 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was conflicting with array-bracket-spacing option "always" on the right side of ,.

Playground link

nzakas
nzakas approved these changes Jul 8, 2022
Copy link
Member

@nzakas nzakas left a comment

LGTM.

@nzakas nzakas merged commit bfe5e88 into main Jul 8, 2022
20 checks passed
@nzakas nzakas deleted the issue16100 branch Jul 8, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jul 18, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.19.0` -> `8.20.0`](https://renovatebot.com/diffs/npm/eslint/8.19.0/8.20.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.20.0`](https://github.com/eslint/eslint/releases/tag/v8.20.0)

[Compare Source](eslint/eslint@v8.19.0...v8.20.0)

#### Features

-   [`ca83178`](eslint/eslint@ca83178) feat: catch preprocess errors ([#&#8203;16105](eslint/eslint#16105)) (JounQin)

#### Bug Fixes

-   [`30be0ed`](eslint/eslint@30be0ed) fix: no-warning-comments rule escapes special RegEx characters in terms ([#&#8203;16090](eslint/eslint#16090)) (Lachlan Hunt)
-   [`bfe5e88`](eslint/eslint@bfe5e88) fix: ignore spacing before `]` and `}` in comma-spacing ([#&#8203;16113](eslint/eslint#16113)) (Milos Djermanovic)

#### Documentation

-   [`845c4f4`](eslint/eslint@845c4f4) docs: Add website team details ([#&#8203;16115](eslint/eslint#16115)) (Nicholas C. Zakas)
-   [`5a0dfdb`](eslint/eslint@5a0dfdb) docs: Link to blog post in no-constant-binary-expression ([#&#8203;16112](eslint/eslint#16112)) (Jordan Eldredge)
-   [`bc692a9`](eslint/eslint@bc692a9) docs: remove install command ([#&#8203;16084](eslint/eslint#16084)) (Strek)
-   [`49ca3f0`](eslint/eslint@49ca3f0) docs: don't show toc when content not found ([#&#8203;16095](eslint/eslint#16095)) (Amaresh  S M)
-   [`ba19e3f`](eslint/eslint@ba19e3f) docs: enhance 404 page UI ([#&#8203;16097](eslint/eslint#16097)) (Amaresh  S M)
-   [`a75d3b4`](eslint/eslint@a75d3b4) docs: remove unused meta.docs.category field in working-with-rules page ([#&#8203;16109](eslint/eslint#16109)) (Brandon Scott)
-   [`cdc0206`](eslint/eslint@cdc0206) docs: add formatters page edit link ([#&#8203;16094](eslint/eslint#16094)) (Amaresh  S M)
-   [`4d1ed22`](eslint/eslint@4d1ed22) docs: preselect default theme ([#&#8203;16098](eslint/eslint#16098)) (Strek)
-   [`4b79612`](eslint/eslint@4b79612) docs: add missing correct/incorrect containers ([#&#8203;16087](eslint/eslint#16087)) (Milos Djermanovic)
-   [`09f6acb`](eslint/eslint@09f6acb) docs: fix UI bug on rules index and details pages ([#&#8203;16082](eslint/eslint#16082)) (Deepshika S)
-   [`f5db264`](eslint/eslint@f5db264) docs: remove remaining duplicate rule descriptions ([#&#8203;16093](eslint/eslint#16093)) (Milos Djermanovic)
-   [`32a6b2a`](eslint/eslint@32a6b2a) docs: Add scroll behaviour smooth ([#&#8203;16056](eslint/eslint#16056)) (Amaresh  S M)

#### Chores

-   [`bbf8df4`](eslint/eslint@bbf8df4) chore: Mark autogenerated release blog post as draft ([#&#8203;16130](eslint/eslint#16130)) (Nicholas C. Zakas)
-   [`eee4306`](eslint/eslint@eee4306) chore: update internal lint dependencies ([#&#8203;16088](eslint/eslint#16088)) (Bryan Mishkin)
-   [`9615a42`](eslint/eslint@9615a42) chore: update formatter examples template to avoid markdown lint error ([#&#8203;16085](eslint/eslint#16085)) (Milos Djermanovic)
-   [`62541ed`](eslint/eslint@62541ed) chore: fix markdown linting error ([#&#8203;16083](eslint/eslint#16083)) (唯然)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTcuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExNy4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1466
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants