-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Disallow padded blocks (blank lines at start/end of block statements) #170
Comments
You shouldn't pad blocks -- you're correct. The eslint rule that enforces this is Unfortunately, we had to disable it in 6f75ec3 because eslint 0.19 started incorrectly reporting errors in code that uses ASI. We can re-enable the rule once this issue (eslint/eslint#2336) is fixed. I'll keep this issue open to remind ourselves to re-enable it once eslint fixes it on their end. |
I just put $10 on the issue. If you're willing, please chip in and they may fix it faster. |
This is fixed in eslint |
This will be fixed in eslint 1.0.0 and standard 5.0.0. #192 |
@feross this is still broken (as in, it doesn't throw a warning) in 5.0.2 😭
|
Confirmed still in function test () {
return 1
}
test() |
This is actually a huge breaking change at this point. 22 out of 138 sample repos fail if we enable this rule. Since this isn't a very important rule -- it doesn't affect code correctness -- I'm inclined to not add this rule back. Or, if we do, it'll have to be in v6.0.0 when/if there are other breaking rules. But I really want |
@feross it was my understanding that this rule was meant to be in place a while ago. I see no reason why we can't just fix up those 22 repositories, the rule has been around long enough that it seems like most users should have been aware of it? It would seem very odd for us to enforce consistent spacing in objects, but not in functions. function a () {
return 1
}
// OK!
function b () {
return 2
}
// OK!
function c () {
return 3
}
// Error! oops |
I would really like to see that rule added back. I can volunteer to submit 22 pull requests to get this moving. As said this rule have been in place for a long time and I think that many people are aware of it, it's easy to miss in one or two places though. Just noticed it in one of my own project, which is what led me to open this issue. Hopefully I can compile a list of the repos and start working on it during this weekend. |
@LinusU Okay, if you get the tests passing, I'm okay with merging this and releasing it in a minor release. |
Okay, 22 pull requests sent! Took me just half an hour, nice 😎 |
@LinusU Second one has just come in 💃 Hyperemitter has updated too. |
Super! 🎆 I've updated the list 👏 |
In terms of @mcollina 's other ones, I'll ping him for access when he gets back. But consider them done. |
@LinusU, awesome work. |
I'm back ;). On this. |
Only two repos to go now 👍 |
@feross There are two holdouts, can we go ahead anyway? |
LGTM |
It's only npm/docs that's holding out now |
Sure, let's go ahead. Released as 5.2.0. |
Cool 👍 |
Could this be reopened until it's documented somewhere (and fixed)? Having to hunt down closed issues to see why the error is there since nothing like this is document is a bit weird... |
@franciscop Padded blocks are disallowed in |
I apologize if there has been an issue on this but I couldn't find it when searching.
I'm very sure that this wasn't allowed earlier, was the change to allow this intentional?
I think it's very good to enforce something here, either always empty line at start of function, or never. Or is there a good reason to sometimes allow it?
I like to think that standard is so good that there will be just one correct output from any AST. Obviously we'll never get quite there, but I believe that being more strict is better.
Bug progress:
The text was updated successfully, but these errors were encountered: