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

Bug: [lines-around-comment] afterHashbangComment option is not supported #6636

Closed
4 tasks done
EvgenyOrekhov opened this issue Mar 14, 2023 · 9 comments
Closed
4 tasks done
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working formatting Related to whitespace/bracket formatting. We strongly recommend you use a formatter instead. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@EvgenyOrekhov
Copy link

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.0.1-rc&sourceType=module&code=MQQgZg9hBQBuCGAnABAE3gT2QXmQIgHNEBTeAFz2iA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6RplWgQ2gHtYTACa0yfALbjELdGADa4bDkTR+0SABpFSrEuyRuAM3wqAEt2QALAEbcmAcwDCEqTIz44ibdgC+3gLqKfj5AA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA

Repro Code

#!foo
var day = "great"

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/lines-around-comment": [
      "error",
      {
        "afterHashbangComment": true
      }
    ]
  },
};

tsconfig

{
  "compilerOptions": {
    // ...
  }
}

Expected Result

Should not crash, should warn about missing blank line after hashbang comment

Actual Result

Configuration for rule "@typescript-eslint/lines-around-comment" is invalid:
Value {"afterHashbangComment":true,"beforeBlockComment":true,"afterBlockComment":false,"beforeLineComment":false,"afterLineComment":false,"allowBlockStart":false,"allowBlockEnd":false} should NOT have additional properties.

Additional Info

The playground example doesn't crash, but CLI crashes.

@EvgenyOrekhov EvgenyOrekhov added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Mar 14, 2023
@IronGeek
Copy link
Contributor

I can confirm this issue.

According to the eslint changelog, the afterHashbangComment options for eslint/lines-around-comment rule is only recently added in v8.35.0

And if we look at the latest https://github.com/typescript-eslint source:

schema: {
type: 'array',
items: [
{
type: 'object',
properties: {
beforeBlockComment: {
type: 'boolean',
default: true,
},
afterBlockComment: {
type: 'boolean',
default: false,
},
beforeLineComment: {
type: 'boolean',
default: false,
},
afterLineComment: {
type: 'boolean',
default: false,
},
allowBlockStart: {
type: 'boolean',
default: false,
},
allowBlockEnd: {
type: 'boolean',
default: false,
},
allowClassStart: {
type: 'boolean',
},
allowClassEnd: {
type: 'boolean',
},
allowObjectStart: {
type: 'boolean',
},
allowObjectEnd: {
type: 'boolean',
},
allowArrayStart: {
type: 'boolean',
},
allowArrayEnd: {
type: 'boolean',
},
allowInterfaceStart: {
type: 'boolean',
},
allowInterfaceEnd: {
type: 'boolean',
},
allowTypeStart: {
type: 'boolean',
},
allowTypeEnd: {
type: 'boolean',
},
allowEnumStart: {
type: 'boolean',
},
allowEnumEnd: {
type: 'boolean',
},
allowModuleStart: {
type: 'boolean',
},
allowModuleEnd: {
type: 'boolean',
},
ignorePattern: {
type: 'string',
},
applyDefaultIgnorePatterns: {
type: 'boolean',
},
},
additionalProperties: false,
},
],
},

We can see that the afterHashbangComment is not yet supported (not yet added to @typescript-eslint/lines-around-comment schema).


On a side note, I just noticed by reading the source, apparently there's more options for the @typescript-eslint/lines-around-comment that is not mentioned in the docs:

  • allowEnumStart
  • allowEnumEnd
  • allowModuleStart
  • allowModuleEnd

Is these options meant to be supported?

@JoshuaKGoldberg JoshuaKGoldberg added formatting Related to whitespace/bracket formatting. We strongly recommend you use a formatter instead. accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Mar 18, 2023
@JoshuaKGoldberg
Copy link
Member

Yup, ESLint core added an afterHashbagComment option that we don't yet have here. Accepting PRs.

more options

Good spot, I'll send a PR to fix up those docs now. Thanks!

@kozlovvski
Copy link
Contributor

kozlovvski commented Mar 20, 2023

@JoshuaKGoldberg I'd like to fix this, however this requires eslint@8.35.0. Since we have "eslint": "^8.15.0" in our package.json, how should I approach this? Should my PR bump the eslint version?

Context:
We use the base eslint rule for the comments in the start/end of the block – we could leverage the same behavior for shebang comments as well. However, we would need the newer version of eslint, which supports the afterHashbangComment option.

@JoshuaKGoldberg
Copy link
Member

Hmm I actually don't know what to do in this situation. @bradzacher?

@bradzacher
Copy link
Member

Given the rule is an extension rule and not a fork - really the schema for the rule should be based off of the base rule's schema and we just add our additional options to the schema. That way we aren't allowing options that aren't supported in the base implementation.

@kozlovvski
Copy link
Contributor

Given the rule is an extension rule and not a fork - really the schema for the rule should be based off of the base rule's schema and we just add our additional options to the schema. That way we aren't allowing options that aren't supported in the base implementation.

Sorry, I am unsure what this means in the afterHashbangComment case. Should the logic work as following?

  1. If the user has a version of eslint supporting the new option, allow this option in the matching typescript-eslint rule and use base rule process to handle it
  2. Otherwise, don't allow the option.

@bradzacher
Copy link
Member

If the user has a version of eslint supporting the new option, allow this option in the matching typescript-eslint rule and use base rule process to handle it

Correct. Currently the schema is hard-coded:

schema: {
type: 'array',
items: [
{
type: 'object',
properties: {
beforeBlockComment: {
type: 'boolean',
default: true,
},
afterBlockComment: {
type: 'boolean',
default: false,
},
beforeLineComment: {
type: 'boolean',
default: false,
},
afterLineComment: {
type: 'boolean',
default: false,
},
allowBlockStart: {
type: 'boolean',
default: false,
},
allowBlockEnd: {
type: 'boolean',
default: false,
},
allowClassStart: {
type: 'boolean',
},
allowClassEnd: {
type: 'boolean',
},
allowObjectStart: {
type: 'boolean',
},
allowObjectEnd: {
type: 'boolean',
},
allowArrayStart: {
type: 'boolean',
},
allowArrayEnd: {
type: 'boolean',
},
allowInterfaceStart: {
type: 'boolean',
},
allowInterfaceEnd: {
type: 'boolean',
},
allowTypeStart: {
type: 'boolean',
},
allowTypeEnd: {
type: 'boolean',
},
allowEnumStart: {
type: 'boolean',
},
allowEnumEnd: {
type: 'boolean',
},
allowModuleStart: {
type: 'boolean',
},
allowModuleEnd: {
type: 'boolean',
},
ignorePattern: {
type: 'string',
},
applyDefaultIgnorePatterns: {
type: 'boolean',
},
},
additionalProperties: false,
},
],
},

Instead of it being hard-coded it should be described in relation to the base rule's schema so that it exposes the same options the base rule does.

@kozlovvski
Copy link
Contributor

Okay, I will try to fix this 🙌🏻

@bradzacher
Copy link
Member

Hi there 👋!

As you may have heard ESLint core is deprecating its formatting rules. We are likewise moving in the same direction and deprecating our formatting rules.

Deprecated rules will be removed in the next major version as their new home is the eslint-stylistic project.

If this issue is important to you please consider filling an issue over at eslint-stylistic which will be the new home for stylistic/formatting rules.

Thanks!

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working formatting Related to whitespace/bracket formatting. We strongly recommend you use a formatter instead. package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
5 participants