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

feat: Add new rule no-empty-static-block #16325

Merged
merged 11 commits into from Nov 6, 2022

Conversation

sosukesuzuki
Copy link
Contributor

If #16318 is accepted, I'll make this PR ready for review.

Prerequisites checklist

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

[ ] Documentation update
[ ] Bug fix (template)
[x] 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:

What changes did you make? (Give an overview)

Fixes #16318

Adds new rule no-empty-static-block

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

@eslint-github-bot eslint-github-bot bot added feature This change adds a new feature to ESLint triage An ESLint team member will look at this issue soon labels Sep 17, 2022
@netlify
Copy link

netlify bot commented Sep 17, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 8f26be5
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63655e48de41760009465ad1

@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 19, 2022
@sosukesuzuki
Copy link
Contributor Author

Is there anything I should do to make progress on this?

@nzakas
Copy link
Member

nzakas commented Oct 25, 2022

@sosukesuzuki if this pull request is ready for review, please click "Ready for Review". We don't leave a lot of feedback on draft PRs.

@sosukesuzuki sosukesuzuki marked this pull request as ready for review October 26, 2022 02:50

## Options

Nothing.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
## Options
Nothing.

This is not needed. If the rule doesn't have any options, this section should be omitted.

type: "suggestion",

docs: {
description: "Disallows empty static blocks",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description: "Disallows empty static blocks",
description: "Disallow empty static blocks",

if (node.body.length === 0 && innerComments.length === 0) {
context.report({
node,
loc: node.loc,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loc: node.loc,

This is redundant, node.loc is used by default if loc is not provided.

const innerComments = sourceCode.getTokens(node, {
includeComments: true,
filter: astUtils.isCommentToken
});
Copy link
Member

Choose a reason for hiding this comment

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

I think that only comments inside { } should count.

class Foo {
  static // this comment should not suppress the error
  {

  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you think about c697d95 ?

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Oct 31, 2022
StaticBlock(node) {
const [blockOpenToken] = sourceCode.getFirstTokens(node, {
filter: token => token.type === "Punctuator" && token.value === "{"
});
const innerComments =
sourceCode.getCommentsInside(node).filter(commentToken => blockOpenToken.range[1] < commentToken.range[0]);

if (node.body.length === 0 && innerComments.length === 0) {
context.report({
node,
messageId: "unexpected"
});
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move token calculations into if (node.body.length === 0) {}, for performance reasons.

It could be something like this:

Suggested change
StaticBlock(node) {
const [blockOpenToken] = sourceCode.getFirstTokens(node, {
filter: token => token.type === "Punctuator" && token.value === "{"
});
const innerComments =
sourceCode.getCommentsInside(node).filter(commentToken => blockOpenToken.range[1] < commentToken.range[0]);
if (node.body.length === 0 && innerComments.length === 0) {
context.report({
node,
messageId: "unexpected"
});
}
}
StaticBlock(node) {
if (node.body.length === 0) {
const closingBrace = sourceCode.getLastToken(node);
if (sourceCode.getCommentsBefore(closingBrace).length === 0) {
context.report({
node,
messageId: "unexpected"
});
}
}
}

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for contributing

@snitin315 snitin315 merged commit 978799b into eslint:main Nov 6, 2022
21 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Nov 10, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.26.0...v8.27.0)

#### Features

-   [`f14587c`](eslint/eslint@f14587c) feat: new `no-new-native-nonconstructor` rule ([#&#8203;16368](eslint/eslint#16368)) (Sosuke Suzuki)
-   [`978799b`](eslint/eslint@978799b) feat: add new rule `no-empty-static-block` ([#&#8203;16325](eslint/eslint#16325)) (Sosuke Suzuki)
-   [`69216ee`](eslint/eslint@69216ee) feat: no-empty suggest to add comment in empty BlockStatement ([#&#8203;16470](eslint/eslint#16470)) (Nitin Kumar)
-   [`319f0a5`](eslint/eslint@319f0a5) feat: use `context.languageOptions.ecmaVersion` in core rules ([#&#8203;16458](eslint/eslint#16458)) (Milos Djermanovic)

#### Bug Fixes

-   [`c3ce521`](eslint/eslint@c3ce521) fix: Ensure unmatched glob patterns throw an error ([#&#8203;16462](eslint/eslint#16462)) (Nicholas C. Zakas)
-   [`886a038`](eslint/eslint@886a038) fix: handle files with unspecified path in `getRulesMetaForResults` ([#&#8203;16437](eslint/eslint#16437)) (Francesco Trotta)

#### Documentation

-   [`ce93b42`](eslint/eslint@ce93b42) docs: Stylelint property-no-unknown ([#&#8203;16497](eslint/eslint#16497)) (Nick Schonning)
-   [`d2cecb4`](eslint/eslint@d2cecb4) docs: Stylelint declaration-block-no-shorthand-property-overrides ([#&#8203;16498](eslint/eslint#16498)) (Nick Schonning)
-   [`0a92805`](eslint/eslint@0a92805) docs: stylelint color-hex-case ([#&#8203;16496](eslint/eslint#16496)) (Nick Schonning)
-   [`74a5af4`](eslint/eslint@74a5af4) docs: fix stylelint error ([#&#8203;16491](eslint/eslint#16491)) (Milos Djermanovic)
-   [`324db1a`](eslint/eslint@324db1a) docs: explicit stylelint color related rules ([#&#8203;16465](eslint/eslint#16465)) (Nick Schonning)
-   [`94dc4f1`](eslint/eslint@94dc4f1) docs: use Stylelint for HTML files ([#&#8203;16468](eslint/eslint#16468)) (Nick Schonning)
-   [`cc6128d`](eslint/eslint@cc6128d) docs: enable stylelint declaration-block-no-duplicate-properties ([#&#8203;16466](eslint/eslint#16466)) (Nick Schonning)
-   [`d03a8bf`](eslint/eslint@d03a8bf) docs: Add heading to justification explanation ([#&#8203;16430](eslint/eslint#16430)) (Maritaria)
-   [`8a15968`](eslint/eslint@8a15968) docs: add Stylelint configuration and cleanup ([#&#8203;16379](eslint/eslint#16379)) (Nick Schonning)
-   [`9b0a469`](eslint/eslint@9b0a469) docs: note commit messages don't support scope ([#&#8203;16435](eslint/eslint#16435)) (Andy Edwards)
-   [`1581405`](eslint/eslint@1581405) docs: improve context.getScope() docs ([#&#8203;16417](eslint/eslint#16417)) (Ben Perlmutter)
-   [`b797149`](eslint/eslint@b797149) docs: update formatters template ([#&#8203;16454](eslint/eslint#16454)) (Milos Djermanovic)
-   [`5ac4de9`](eslint/eslint@5ac4de9) docs: fix link to formatters on the Core Concepts page ([#&#8203;16455](eslint/eslint#16455)) (Vladislav)
-   [`33313ef`](eslint/eslint@33313ef) docs: core-concepts: fix link to semi rule ([#&#8203;16453](eslint/eslint#16453)) (coderaiser)

</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, check this box

---

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

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1628
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 contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Rule: no-empty-static-block
5 participants