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

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

Projects

None yet

6 participants

@LinusU
Collaborator
LinusU 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
Owner
feross 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 from Did we start allowing blank lines in the beginning of files and functions? to Disallow padded blocks (blank lines at start/end of block statements) Jun 24, 2015
@feross
Owner
feross 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
Owner
feross commented Jul 13, 2015

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

@feross
Owner
feross 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
Collaborator
LinusU 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
Collaborator

Confirmed still in 5.0.2.

function test () {

  return 1

}

test()
@dcousens dcousens referenced this issue in JedWatson/react-select Aug 13, 2015
Merged

Searching text prop (Rebased #141) #379

@feross
Owner
feross 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
Collaborator

@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
Collaborator
LinusU 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
Owner
feross 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
Collaborator
LinusU commented Aug 14, 2015

Okay, 22 pull requests sent! Took me just half an hour, nice ๐Ÿ˜Ž

@LinusU
Collaborator
LinusU 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

@LinusU Second one has just come in ๐Ÿ’ƒ Hyperemitter has updated too.

@LinusU
Collaborator
LinusU commented Aug 14, 2015

Super! ๐ŸŽ†

I've updated the list ๐Ÿ‘

@mcdonnelldean

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

@dcousens
Collaborator

@LinusU, awesome work.

@mcollina

I'm back ;). On this.

@LinusU
Collaborator
LinusU commented Aug 19, 2015

Only two repos to go now ๐Ÿ‘

@LinusU
Collaborator
LinusU commented Aug 31, 2015

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

@dcousens
Collaborator

LGTM

@LinusU
Collaborator
LinusU commented Aug 31, 2015

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

@feross
Owner
feross commented Sep 3, 2015

Sure, let's go ahead.

Released as 5.2.0.

@feross feross closed this Sep 3, 2015
@feross feross added a commit that referenced this issue Sep 3, 2015
@feross test: disable npm/docs for #170 8a52459
@LinusU
Collaborator
LinusU commented Sep 3, 2015

Cool ๐Ÿ‘

@franciscop

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment