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

New rules: Disallow or enforce spaces inside of curly braces and brackets (object-curly-spacing and array-bracket-spacing) #609

Closed
ischas opened this issue Sep 1, 2016 · 19 comments

Comments

@ischas
Copy link

ischas commented Sep 1, 2016

http://eslint.org/docs/rules/object-curly-spacing

http://eslint.org/docs/rules/array-bracket-spacing

Standard JS should define if there should be spaces inside of brackets and curly braces or not.

By now, following code is allowed but looks awkward:

import { foo } from 'bar';
import {foo} from 'bar';
let a = [1, 2]
let a = [ 1, 2 ]
var obj = {foo: 'bar'};
var obj = { foo: 'bar' };

Personally I have no preference if there should be spaces or not.

@feross feross added this to the standard v9 milestone Sep 10, 2016
@feross
Copy link
Member

feross commented Sep 10, 2016

Agreed.

Now that this is automatically fixable with standard --fix, we can tighten up this rule in standard v9. There is already existing discussion about this here: standard/eslint-config-standard#35

@feross feross modified the milestones: standard v9, standard v10 Feb 9, 2017
@feross feross modified the milestones: standard v10, standard v11 Mar 2, 2017
@capaj
Copy link
Contributor

capaj commented May 4, 2017

I personally prefer without spaces. The reason is saving editor space-the line has only 80 characters and even two spaces can make a difference whether the line is overflowing or not.

Many people would argue that it's more apparent when the braces are spaced-to those I would say-configure your editor to highlight them.
Here's my editor:

image

I use a plugin https://marketplace.visualstudio.com/items?itemName=2gua.rainbow-brackets to make them more apparent and that works out better than trying to use spaces.

@langri-sha
Copy link

langri-sha commented May 6, 2017

🗒 There's already a ban on spaces within curly braces inside of template strings.

@eddiemonge
Copy link

All the documentation shows spaces in object in the rules list so that should be whats used since it looks like its implied that it is.

@flippidippi
Copy link

@feross v9 is already behind us, really wish we could decide on this. I think this is the biggest part missing in standard and it's driving me nuts because there's still no consistency. In standard/eslint-config-standard#35 it looks like most people are leaning towards spaces around objects and not arrays.

@langri-sha
Copy link

@LinusU
Copy link
Member

LinusU commented Jun 27, 2018

I really think it's time that we take a decision on this! This is something that constantly creeps up during code review for me, and the promise of Standard was that that shouldn't happen. Let's fix this 🙌

Here is the current eco-system impact:

"object-curly-spacing": ["error", "always"]

# tests 271
# pass  192
# fail  79
# violations 1935
"object-curly-spacing": ["error", "never"]

# tests 271
# pass  158
# fail  113
# violations 3510

I think we should just go with always because 1) it seems to be what the community already does, and 2) it's how we format it in the readme of this repo.

I'll send a PR now 🚀

LinusU added a commit to standard/eslint-config-standard that referenced this issue Jun 27, 2018
Per discussion [here](standard/standard#609 (comment)) (standard/standard#609 (comment)) it seems like it's both time for this to happen, and that the community prefers `"always"`.
@dougwilson
Copy link

FWIW as I have been using standard I have gone back and forth between never and always since standard allows both. Over time I have ended up with the always style.

@damanwiththeplan
Copy link

damanwiththeplan commented Jul 5, 2018

This should be added as always, just because it's simply more readable and more consistent within object declarations.

a:
{foo: "bar"}
b:
{ foo: "bar" }

there is already a space after : it looks less awkward if u add all the spaces

@feross feross modified the milestones: standard v12, standard v13 Aug 28, 2018
@feross
Copy link
Member

feross commented Aug 28, 2018

Thank you to everyone for sharing your thoughts on this issue. Thanks for sending the PR, @LinusU! Let's merge it and put this one behind us! 🙌

@feross feross closed this as completed Aug 28, 2018
@noygal
Copy link

noygal commented Sep 3, 2018

Hi, first of all thank for all the effort you put into this project, I've been using it for years now (before that I was on "npm style guide") and I love the minimalist code it enforce.

With regard to this rule addition to standard you render your tool useless for almost all my projects and other developers that use javascript functional programming approach. The minimal object notation reduce a lot of space on one liners and make the functional flow clearer.
Consider this code:

const foo = ({in1, in2}) => ({out1: in1.reduce((acc, el, i) => (acc[el] = {x: in2[i], y: i}) && acc, {})})

This is conventional functional code example, readability is a matter of opinion, but I think that most would agreed that spacing out the objects won't make it any clearer.

Just to clear that this not an opinionated view, check the docs for Ramda and Bobab - "the granddaddy" of functional programming in javascript:
https://ramdajs.com/docs/
https://github.com/Yomguithereal/baobab

@LinusU
Copy link
Member

LinusU commented Sep 3, 2018

With regard to this rule addition to standard you render your tool useless for almost all my projects and other developers that use javascript functional programming approach.

I don't think that this is true. I use functional programming a lot, and have no problems with having spaces in the curly objects.

and make the functional flow clearer.

I'm not sure I agree with this, the flow will still be exactly the same? 🤔

I personally think that the spacing makes it more clear where the objects begin and end, which I think makes it easier to see where the definition of out1 ends in this example:

const foo = ({in1, in2}) => ({out1: in1.reduce((acc, el, i) => (acc[el] = {x: in2[i], y: i}) && acc, {})})
const foo = ({ in1, in2 }) => ({ out1: in1.reduce((acc, el, i) => (acc[el] = { x: in2[i], y: i }) && acc, {}) })

Ultimately, we are of course going to upset a few people with this change, but I hope that you can see the benefit of unifying behind a single standard. As our testing indicated, most people already used spacing, and that's why we went with that...

@noygal
Copy link

noygal commented Sep 3, 2018

As our testing indicated, most people already used spacing, and that's why we went with that...

With that approach we should have stayed with semicolon 😄

But honestly, I really can't argue with the your decisions regarding the future of standard, just to note that as a long time community user I find this decision as contradiction to the reasons I choose to use standard.
Forcing the user to add extra spacing is non minimalistic in nature and in the past was done on when it serve a clear purpose: like in the extra space on class method declaration, if anything I would've expected that standard to force the removal of the redundant spacing considering standard past decisions.

For now I'll stay on v11, but I hope that more people would raised concerns about this issue.

@LinusU
Copy link
Member

LinusU commented Sep 3, 2018

With that approach we should have stayed with semicolon

I was referring to the tests I ran on existing packages using standard, so no semicolons there 😄

Forcing the user to add extra spacing is non minimalistic in nature [...]

I'm not sure that we are aiming for minimalistic, the goal is to increase readability not remove as much as possible. e.g. this is still valid JS, but not too easy to read:

const foo=({in1, in2})=>({out1:in1.reduce((acc,el,i)=>(acc[el]={x:in2[i],y:i})&&acc,{})})

For now I'll stay on v11, but I hope that more people would raised concerns about this issue.

Sounds like a good plan for now, if there is much push back on this we can of course reconsider. Although I don't think that many people will have a problem with it ☺️

@flippidippi
Copy link

@LinusU It looks like we addressed the object-curly-spacing but the array-bracket-spacing is still not decided? We currently use standardx just to enforce these:

 "object-curly-spacing": ["error", "always"],
 "array-bracket-spacing": ["error", "never"]

@LinusU
Copy link
Member

LinusU commented Sep 6, 2018

@flipxfx oh, I thought we already were using "never" in standard as well 😄 it would have been nice to introduce them both together.

Let's run the test and see how breaking it would be to add, as far as I've seen, it's not common to add spaces to arrays.

If you want to run the ecosystem impact test that would be much appreciated ☺️

  1. Clone this repo
  2. Edit eslintrc.json
  3. Run npm it

@flippidippi
Copy link

{
  "extends": ["standard", "standard-jsx"],
  "rules": {
    "array-bracket-spacing": ["error", "never"]
  }
}

# tests 187
# pass  159
# fail  28
{
  "extends": ["standard", "standard-jsx"],
  "rules": {
    "array-bracket-spacing": ["error", "always"]
  }
}

# tests 187
# pass  109
# fail  78

@flippidippi
Copy link

@LinusU should we create a new issue for this? It would be a simple fix and it seems "array-bracket-spacing": ["error", "never"] would be the way to go.

@LinusU
Copy link
Member

LinusU commented Sep 21, 2018

Yes please, sounds good 👍

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