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

Comment at end of block before closing } trigger padded-block 'never' rule. #2336

Closed
wraithan opened this issue Apr 17, 2015 · 13 comments
Closed
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@wraithan
Copy link

It appears when you have a comment at the end of a block it is still triggering the padded-block rule when set to 'never'. This is on latest published at time of this issue (v0.19.0). I've included the output, the example, the version command, and our eslintrc below. Let me know if any more information would be helpful.

Command line output:

eslint test.js

test.js
  6:1  error  Block must not be padded by blank lines  padded-blocks

Example JS (test.js):

a()

function a () {
  a()
  // stuff
}

eslint version verification:

eslint --version
v0.19.0

Our .eslintrc:

{
  "env": {
    "node": true,
    "browser": false
  },
  "global": [
    "after",
    "afterEach",
    "before",
    "beforeEach",
    "describe",
    "it"
  ],
  "rules": {
    "brace-style": 2,
    "comma-style": [2, "last"],
    "consistent-return": 0,
    "curly": 0,
    "eol-last": 2,
    "eqeqeq": 2,
    "camelcase": 0,
    "dot-notation": 2,
    "func-names": 2,
    "guard-for-in": 2,
    "max-len": [2, 90],
    "max-nested-callbacks": [2, 3],
    "max-params": [2, 5],
    "new-cap": 2,
    "no-comma-dangle": 0,
    "no-console": 1,
    "no-else-return": 2,
    "no-floating-decimal": 2,
    "no-lonely-if": 2,
    "no-mixed-requires": 2,
    "no-multiple-empty-lines": 2,
    "no-new": 2,
    "no-shadow": 1,
    "no-undef": 2,
    "no-underscore-dangle": 0,
    "no-unused-vars": 2,
    "no-use-before-define": 0,
    "padded-blocks": [2, "never"],
    "radix": 2,
    "semi": [2, "never"],
    "space-after-function-name": 0,
    "space-after-keywords": 2,
    "space-before-blocks": 2,
    "space-infix-ops": 2,
    "spaced-line-comment": 2,
    "space-return-throw-case": 2,
    "space-unary-ops": 2,
    "strict": 0,
    "quotes": [0, "single"],
    "wrap-iife": 2
  }
}

Past similar issues: #1938

@lo1tuma
Copy link
Member

lo1tuma commented Apr 17, 2015

This could probably be bug in espree, but I’m not 100% sure how the comment attachment algorithm is supposed to work.

The problem with your example is, that it relies on ASI and the trailing comment is not attached to the ExpressionStatement.

@lo1tuma lo1tuma added bug ESLint is working incorrectly rule Relates to ESLint's core rules labels Apr 17, 2015
@nzakas
Copy link
Member

nzakas commented Apr 17, 2015

Comment attachment does get confused by ASI - I'm not sure if that's a bug or a feature. :)

@feross
Copy link
Contributor

feross commented Jun 24, 2015

I'm willing to put $10 on this issue.

There's no BountySource link for some reason (perhaps it's another BountySource bug), but I can pay the person who solves it directly through Paypal or Bitcoin.

@LinusU
Copy link
Contributor

LinusU commented Jun 24, 2015

I'm willing to chip in $10 as well, I can pay with Bitcoin.

@nzakas
Copy link
Member

nzakas commented Jun 24, 2015

Sorry, the Bountysource plugin is broken. I've contacted them about it but haven't gotten any useful information.

I'd ideally like to solve this in the parser, but if this is a big pain point, I think @lo1tuma had a solution that didn't require a parser change. We can use that until the parser is fixed to eliminate the pain

@feross
Copy link
Contributor

feross commented Jul 6, 2015

@lo1tuma Is your solution still viable? If it's not too onerous, it might be nice to use it until there's a chance to fix the parser, as @nzakas said.

@lo1tuma
Copy link
Member

lo1tuma commented Jul 7, 2015

@feross My proposed solution is a workaround which probably decreases the performance of the rule. I think it would also fix #2788 (I can add a test to verify this). If we want to merge this until the comment attachment algorithm is fixed in espree we should file a new issue to remove the workaround in the future.

@nzakas
Copy link
Member

nzakas commented Jul 7, 2015

Yeah, let's do that. I'm not sure when I'll be up to digging into the comment attachment stuff, it's mind-bending and my mind isn't too flexible ATM.

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Jul 7, 2015
ilyavolodin added a commit that referenced this issue Jul 11, 2015
Fix: fix comment handling in padded-blocks when relying on ASI (fixes #2336)
@feross
Copy link
Contributor

feross commented Jul 13, 2015

@lo1tuma, where should I send the bounty I promised for this issue? Paypal, Bitcoin, Venmo works.

cc @LinusU

@LinusU
Copy link
Contributor

LinusU commented Jul 14, 2015

I'm on! Bitcoin (or BountySource) would work best for me 🎆

@lo1tuma
Copy link
Member

lo1tuma commented Jul 15, 2015

@feross, @LinusU: Thanks, but I’m not interested in earning money with my contributions, so feel free to keep to money or donate it directly to the eslint team via BountySource.

@feross
Copy link
Contributor

feross commented Jul 15, 2015

@lo1tuma Donated to the eslint team on BountySource - thanks!

@LinusU
Copy link
Contributor

LinusU commented Jul 15, 2015

screen shot 2015-07-15 at 23 44 05
👍

Flet pushed a commit to Flet/standard that referenced this issue Oct 23, 2015
Since eslint 0.19, this rule is broken on code that uses ASI. Disabling
the rule until this issue
(eslint/eslint#2336) is fixed.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 7, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

5 participants