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

Disallow padded blocks (blank lines at start/end of block statements) #170

Closed
LinusU opened this Issue Jun 24, 2015 · 25 comments

Comments

6 participants
@LinusU
Copy link
Member

commented Jun 24, 2015

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?

function test () {

  return 12
}

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:

  • ahmadnassri/echint
  • ahmadnassri/forwarded-http
  • ahmadnassri/har-validator
  • ahmadnassri/har-expander
  • feross/buffer
  • feross/bittorrent-tracker
  • feross/typedarray-to-buffer
  • feross/StudyNotes
  • feross/webtorrent-www
  • feross/whiteboard
  • feross/webtorrent
  • Flet/standard-engine
  • maxogden/dat-core
  • maxogden/requirebin
  • maxogden/dat
  • maxogden/torrent
  • mcollina/fastfall
  • mcollina/fastq
  • mcollina/hyperemitter
  • moose-team/friends
  • motdotla/dotenv
  • npm/docs
@feross

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

You shouldn't pad blocks -- you're correct. The eslint rule that enforces this is padded-blocks.

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.

@feross feross changed the title Did we start allowing blank lines in the beginning of files and functions? Disallow padded blocks (blank lines at start/end of block statements) Jun 24, 2015

@feross feross added the blocked label Jun 24, 2015

@feross

This comment has been minimized.

Copy link
Member

commented Jun 24, 2015

I just put $10 on the issue. If you're willing, please chip in and they may fix it faster.

eslint/eslint#2336

@feross feross added the bug label Jun 28, 2015

@feross

This comment has been minimized.

Copy link
Member

commented Jul 13, 2015

This is fixed in eslint master, but we need to wait for an npm release.

@feross

This comment has been minimized.

Copy link
Member

commented Jul 17, 2015

This will be fixed in eslint 1.0.0 and standard 5.0.0. #192

@feross feross closed this Jul 23, 2015

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2015

@feross this is still broken (as in, it doesn't throw a warning) in 5.0.2 😭

➜  /tmp  cat test.js     
function test () {

  return 1

}

test()
➜  /tmp  standard --version
5.0.2
➜  /tmp  standard test.js
➜  /tmp  

@dcousens dcousens reopened this Aug 13, 2015

@dcousens

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

Confirmed still in 5.0.2.

function test () {

  return 1

}

test()
@feross

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

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 standard to be "done" at some point, so we can stop futzing with the rules and just be productive (which was kinda the whole point of this module ;) Ideally, there won't be a v6.0.0 for a long time.

@dcousens

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

@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.
I'd almost be OK with it if we were enforcing:

function a () {
  return 1
}
// OK!

function b () {

  return 2

}
// OK!

function c () {

  return 3
}
// Error! oops
@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

I would really like to see that rule added back. I can volunteer to submit 22 pull requests to get this moving.

200

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.

@feross

This comment has been minimized.

Copy link
Member

commented Aug 14, 2015

@LinusU Okay, if you get the tests passing, I'm okay with merging this and releasing it in a minor release.

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

Okay, 22 pull requests sent! Took me just half an hour, nice 😎

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

First merged! Only 21 to go, I'll edit the first post so that it shows the progress on the issue

screen shot 2015-08-14 at 22 57 57

@mcdonnelldean

This comment has been minimized.

Copy link

commented Aug 14, 2015

@LinusU Second one has just come in 💃 Hyperemitter has updated too.

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2015

Super! 🎆

I've updated the list 👏

@mcdonnelldean

This comment has been minimized.

Copy link

commented Aug 14, 2015

In terms of @mcollina 's other ones, I'll ping him for access when he gets back. But consider them done.

@dcousens

This comment has been minimized.

Copy link
Member

commented Aug 15, 2015

@LinusU, awesome work.

@mcollina

This comment has been minimized.

Copy link

commented Aug 19, 2015

I'm back ;). On this.

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2015

Only two repos to go now 👍

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

@feross There are two holdouts, can we go ahead anyway?

@dcousens

This comment has been minimized.

Copy link
Member

commented Aug 31, 2015

LGTM

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2015

It's only npm/docs that's holding out now

@feross

This comment has been minimized.

Copy link
Member

commented Sep 3, 2015

Sure, let's go ahead.

Released as 5.2.0.

@feross feross closed this Sep 3, 2015

feross added a commit that referenced this issue Sep 3, 2015

@LinusU

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2015

Cool 👍

@franciscop

This comment has been minimized.

Copy link

commented May 16, 2016

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...

@feross

This comment has been minimized.

Copy link
Member

commented May 16, 2016

@franciscop Padded blocks are disallowed in standard. Not sure what your issue is. If you have a new bug to report, please open a new issue.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 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.