8000 [indent] parameter with decorator wrongly indented · Issue #1232 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

[indent] parameter with decorator wrongly indented #1232

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.

Sign up for GitHub

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

Jump to bottom
Closed
edward-soetiono-asto opened this issue Nov 19, 2019 · 4 comments
Closed

[indent] parameter with decorator wrongly indented #1232

edward-soetiono-asto opened this issue Nov 19, 2019 · 4 comments
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Milestone
indent rewrite

Comments

@edward-soetiono-asto
Copy link

Repro

{
  "extends": ["airbnb-typescript/base", "prettier"],
  "parser": "@typescript-eslint/parser",
  "plugins": ["import", "@typescript-eslint", "prettier"],
  "rules": {
    "import/prefer-default-export": "off",
    "max-classes-per-file": "off",
    "class-methods-use-this": "off",
    "import/no-unresolved": "error"
  },
  "settings": {
    "import/parsers": {
      "@typescript-eslint/parser": [".ts"]
    },
    "import/resolver": {
      "typescript": {}
    }
  }
}

@Post('/')
@OnUndefined(201)
async someFunction(
  @CurrentUser({ required: true }) user: User,
  @Body({ required: true }) body: SomeRequest
): Promise<void> {
  // function truncated
}

Expected Result

@Post('/')
@OnUndefined(201)
async someFunction(
  @CurrentUser({ required: true }) user: User,
  @Body({ required: true }) body: SomeRequest
): Promise<void> {
  // function truncated
}

Actual Result

@Post('/')
@OnUndefined(201)
async someFunction(
  @CurrentUser({ required: true }) user: User,
    @Body({ required: true }) body: SomeRequest
): Promise<void> {
  // function truncated
}

Additional Info

The 2nd parameter @Body was wrongly indented 6 spaces ( should be 4 spaces).

Works in @typescript-eslint/eslint-plugin and @typescript-eslint/parser 2.7.0

Versions

package version
@typescript-eslint/eslint-plugin 2.8.0
@typescript-eslint/parser 2.8.0
TypeScript 3.7.2
ESLint 6.6.0
@edward-soetiono-asto edward-soetiono-asto added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Nov 19, 2019
@edward-soetiono-asto edward-soetiono-asto changed the title [@typescript-eslint/indent] <issue title> [@typescript-eslint/indent] parameter with decorator wrongly indented in 2.8.0 Nov 19, 2019
@bradzacher bradzacher changed the title [@typescript-eslint/indent] parameter with decorator wrongly indented in 2.8.0 [indent] parameter with decorator wrongly indented in 2.8.0 Nov 19, 2019
@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for team members to take a look labels Nov 19, 2019
@bradzacher bradzacher added this to the indent rewrite milestone Nov 19, 2019
@bradzacher bradzacher changed the title [indent] parameter with decorator wrongly indented in 2.8.0 [indent] parameter with decorator wrongly indented Nov 19, 2019
@sgarner
Copy link
sgarner commented Nov 30, 2019

Looks like this bug was an unwanted side effect introduced by #1189.

Using the following configuration:

{
  "rules": {
    "@typescript-eslint/indent": ["error", 2]
  }
}

✅ Under eslint-plugin@=2.7.0 the following indentation passes linting, as desired:

export default class CreateMigration extends Command {
  public execute(
    @param({ description: 'Migration name', required: true, type: String })
      name: string,
    @param({ description: 'Timestamp (optional, default will use current time)', required: false, type: Number })
      timestamp?: number,
  ): void {
    return;
  }
}

❌ Under eslint-plugin@^2.8.0 the following error is thrown:

  5:1  error  Expected indentation of 6 spaces but found 4  @typescript-eslint/indent

The indentation it expects looks, nonsensically, like the following:

export default class CreateMigration extends Command {
  public execute(
    @param({ description: 'Migration name', required: true, type: String })
      name: string,
      @param({ description: 'Timestamp (optional, default will use current time)', required: false, type: Number })
      timestamp?: number,
  ): void {
    return;
  }
}

It is not allowed to have the decorators aligned with the parameters either, as in:

export default class CreateMigration extends Command {
  public execute(
    @param({ description: 'Migration name', required: true, type: String })
    name: string,
    @param({ description: 'Timestamp (optional, default will use current time)', required: false, type: Number })
    timestamp?: number,
  ): void {
    return;
  }
}

❌ This produces the following errors:

  4:1  error  Expected indentation of 6 spaces but found 4  @typescript-eslint/indent
  5:1  error  Expected indentation of 6 spaces but found 4  @typescript-eslint/indent
  6:1  error  Expected indentation of 6 spaces but found 4  @typescript-eslint/indent

@bradzacher
Copy link
Member
bradzacher commented Nov 30, 2019 •
edited
Loading

Happy to accept a PR.
There's good reasons that the indent rule is buggy and problematic:

  1. None of the maintainers use it, because we all use prettier.
  2. Our extension implementation monkey patches the base rule, and does it poorly.
  3. We monkey patch the rule, because the original implementation is 1600 lines of complexity.

I looked into converting the rule to a first-class implementation (hence this unused code exists in the package), but as per reasons (1) and (3), it's more work than I have time to do.

I've always offered the same response:
This is a community run project, and we rely upon community contributions. The core maintainers are volunteers, and use their time mostly on moderating and reviewing.
If you (or someone else) cares about this rule enough, happy to accept a PR to either fix the monkey patch, or to make the rule a firs 8000 t-class citizen.

@bradzacher bradzacher mentioned this issue Dec 19, 2019
Closed
@fkleuver fkleuver mentioned this issue Jan 2, 2020
Merged
@colthreepv
Copy link

I agree original implementation it's a bit crazy, I think all the turmoil regarding this rule is that, everyone tries not to disable it, because it is responsible to indent the code of your entire codebase

Maybe it would be just ok to start from scratch supporting very few use cases for indent, scrapping the original eslint one?

@jaxoncreed jaxoncreed mentioned this issue Feb 19, 2020
Merged
@evilPaprika evilPaprika mentioned this issue Mar 26, 2020
Merged
@bradzacher
Copy link
Member

Merging into #1824

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
indent rewrite
Development

No branches or pull requests
4 participants
@sgarner @colthreepv @bradzacher @edward-soetiono-asto
0