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
21 of 22 tasks
LinusU opened this issue Jun 24, 2015 · 25 comments
Closed
21 of 22 tasks

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

LinusU opened this issue Jun 24, 2015 · 25 comments

Comments

@LinusU
Copy link
Member

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
Copy link
Member

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 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
Copy link
Member

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
Copy link
Member

feross commented Jul 13, 2015

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

@feross
Copy link
Member

feross commented Jul 17, 2015

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

@feross feross closed this as completed Jul 23, 2015
@LinusU
Copy link
Member Author

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
Copy link
Member

Confirmed still in 5.0.2.

function test () {

  return 1

}

test()

@feross
Copy link
Member

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

LinusU commented Aug 14, 2015

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

@LinusU
Copy link
Member Author

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
Copy link

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

@LinusU
Copy link
Member Author

LinusU commented Aug 14, 2015

Super! 🎆

I've updated the list 👏

@mcdonnelldean
Copy link

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

@dcousens
Copy link
Member

@LinusU, awesome work.

@mcollina
Copy link

I'm back ;). On this.

@LinusU
Copy link
Member Author

LinusU commented Aug 19, 2015

Only two repos to go now 👍

@LinusU
Copy link
Member Author

LinusU commented Aug 31, 2015

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

@dcousens
Copy link
Member

LGTM

@LinusU
Copy link
Member Author

LinusU commented Aug 31, 2015

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

@feross
Copy link
Member

feross commented Sep 3, 2015

Sure, let's go ahead.

Released as 5.2.0.

@feross feross closed this as completed Sep 3, 2015
feross added a commit that referenced this issue Sep 3, 2015
@LinusU
Copy link
Member Author

LinusU commented Sep 3, 2015

Cool 👍

@franciscop
Copy link

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
Copy link
Member

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.

@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.
Projects
Archived in project
Development

No branches or pull requests

6 participants