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

padded-blocks fails on comment #2788

Closed
keradus opened this issue Jun 18, 2015 · 12 comments

Comments

Projects
None yet
4 participants
@keradus
Copy link

commented Jun 18, 2015

Hi. When I have a code:

if (a) {
    // we really need to test it
    if (b) {
        c();
    }

    d();
}

The rule warns me: Block must not be padded by blank lines padded-blocks.
It should not since comment is not blank line.

When I remove a comment then rule passes.

@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

Can you share your eslint configuration? Do you use the default parser?

@keradus

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

Default parser it is.

{
    "rules": {
        "padded-blocks": [2, "never"]
    }
}
@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

You disabled the rule in your configuration. So it should never warn.

However, I've tried your example in the online demo and can’t reproduce the problem:

/* eslint padded-blocks: [2, "never"] */

if (a) {
    // we really need to test it
    if (b) {
        c();
    }

    d();
}

Which version of ESLint do you use?

@lo1tuma lo1tuma added the triage label Jun 18, 2015

@keradus

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

"padded-blocks": 0, // S3: docelowo [2, "never"],
yeah, error occurs only with commented setting. I need to disable it due to that problem. Sorry for that.

ESLint version: 0.22.1

@keradus

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

same on 0.23.0

@keradus

This comment has been minimized.

Copy link
Author

commented Jun 18, 2015

Please try this

/* eslint padded-blocks: [2, "never"] */

var xxx = function () {
    // foo
    if (
        // bar
        a ||
        // baz
        b
    ) {
        return;
    }
};
@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

Now I can reproduce it.

Here is more minimal example:

/* eslint padded-blocks: [2, "never"] */

{
    // comment
    if (// comment
        a) {}
}

This is a problem with the comment attachment algorithm of espree. Both comments are getting attached to the Identifier node inside the if condition. The current implementation of the rule would expect that the first comment gets attached to the IfStatement node.

@lo1tuma lo1tuma added bug rule accepted and removed triage labels Jun 18, 2015

@nzakas

This comment has been minimized.

Copy link
Member

commented Jun 18, 2015

Can you file an espree bug?

@moll

This comment has been minimized.

Copy link

commented Jun 20, 2015

Just stumbled upon this too. Same issue with trailing comments.

function foo() {
  console.log("Hi")

  // Long text.
  // Long text.
  // Long text.
}
@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jun 20, 2015

@moll This is a different issue caused by the missing semicolon after console.log("Hi"), see #2336.

@lo1tuma

This comment has been minimized.

Copy link
Member

commented Jul 5, 2015

For reference eslint/espree#155

@keradus

This comment has been minimized.

Copy link
Author

commented Jul 13, 2015

Great to have that fixed! Waiting for next stable release ;)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.