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

Object curlies: File/project–wide or standard? #35

Closed
mightyiam opened this Issue Jun 5, 2016 · 72 comments

Comments

@mightyiam
Copy link
Contributor

mightyiam commented Jun 5, 2016

So, we got #21 and that's great.

It got us instance–wide consistency.

Now, what about file–wide consistency, as was mentioned here?

And, now that I'm thinking about it, why not project–wide consistency?

And thinking further, why not choose a style as standard?

Either no spaces ({a, b}) or a single space ({ a, b }).

@timdp

This comment has been minimized.

Copy link

timdp commented Jul 7, 2016

Personally, I've been struggling with the choice between {foo, bar} and { foo, bar } for a while. I think the former makes more sense because I also write [foo, bar], but it feels like I'm seeing the latter more often. Ultimately, I'm fine with either, but I'm basically expecting Standard to decide for me. Just my two cents.

@cayuu

This comment has been minimized.

Copy link

cayuu commented Jul 8, 2016

Agreed. I've seen a lot of inconsistency in a multi-person Standard project with different bikeshed preferences ensuring both styles {a} and { a } are being utilised in different files. When team {a} players land on { a } files (and vice versa), I'm seeing commits that do nothing other than change the spacing. 😜

We've rolled a custom rule to prevent this. But like, so non-Standard.

Also don't care which style prevails, but picking one seems squarely in Standard's decision space.

@PrototypeAlex

This comment has been minimized.

Copy link

PrototypeAlex commented Jul 10, 2016

I thought I was alone on struggling to pick a side on this, but it seems the lack of clear choice is quite common.

Is the hard part what @timdp pointed out?
The fact that choosing a rule for object culries might force a rule for arrays? or can both live in harmony being different from one another?

const array = [one, two];
const object = { one, two }; 
const array = [one, two];
const object = {one, two}; 
const array = [ one, two ];
const object = { one, two }; 
@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Jul 11, 2016

@feross, it might be time to "just pick something" for the next version IMHO. I'd be happy with either:

const array = [one, two];
const object = { one, two }; 
const array = [one, two];
const object = {one, two}; 

But, I don't mind as long as it is enforced.

@jokeyrhyme

This comment has been minimized.

Copy link
Contributor

jokeyrhyme commented Jul 15, 2016

Apparently the --fix option works: http://eslint.org/docs/rules/object-curly-spacing

Surely the fact that compliance can be automated like this makes this a less controversial change? Why would you bring 1 or 2 files into compliance rather than the whole project, when doing the whole project is this simple?

@feross

This comment has been minimized.

Copy link
Member

feross commented Jul 15, 2016

@dcousens I'm fine with going with your latter example, since that's more consistent. I usually add spaces around my objects, but consistency is more important. If --fix can handle it, then it's probably fine to just ship this.

@timdp

This comment has been minimized.

Copy link

timdp commented Jul 15, 2016

Looks like I'm going to be fixing a lot of scripts soon, but all the better! 👍

@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Jul 15, 2016

@feross my only aversion to the latter is

let map = {a: 1}

instead of

let map = { a: 1 }

I think the spacing keeps up a form of spacial difference, where the tightly packed array allows you immediately recognize it. Obviously subjective.

Removing padding spaces from object definitions might actually lead to mistakes since it looks closer to an array.
Maybe.

@feross

This comment has been minimized.

Copy link
Member

feross commented Jul 15, 2016

You raise a good point. This is definitely very subjective. 😅

Btw, it would be:

let map = {a: 1}

Not:

let map = {a:1}
@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Jul 15, 2016

I wonder if either is "easier" to search for?
I have fallen in love with function myfunc () {} for that reason alone.

@dcousens dcousens added the question label Jul 15, 2016

@cayuu

This comment has been minimized.

Copy link

cayuu commented Jul 15, 2016

{ a: 1 } is more readable, {a: 1} is more consistent (with other single line enclosure syntax e.g.: [x, ..], (x, ..)). This is pure convention, so either choice is "valid". But if consistency is the yardstick, the tight wrap has an edge.

@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Jul 15, 2016

Agreed with @cayuu.
But I'd add a point from my post above.
If safety from errors/mistakes is the yardstick. I think { a: 1 } has the edge.

@lolmaus

This comment has been minimized.

Copy link

lolmaus commented Aug 9, 2016

This line:

import {belongsTo/*, hasMany*/} from 'ember-data/relationships'

triggers this error:

Expected consistent spacing (standard/object-curly-even-spacing)

I believe it should be considered valid: it contains no stray whitespace; a comment is not whitespace.

@lolmaus

This comment has been minimized.

Copy link

lolmaus commented Aug 9, 2016

Should I file a separate issue? Into which repo?

@mightyiam

This comment has been minimized.

Copy link
Contributor Author

mightyiam commented Aug 9, 2016

@lolmaus I would file it against this repo.

@lolmaus

This comment has been minimized.

Copy link

lolmaus commented Aug 9, 2016

@mightyiam The offending rule is standard/object-curly-even-spacing. I believe object-curly-spacing does not have that effect.

@mightyiam

This comment has been minimized.

Copy link
Contributor Author

mightyiam commented Aug 9, 2016

Woops

@feross

This comment has been minimized.

Copy link
Member

feross commented Aug 10, 2016

@lolmaus Yes, this is a separate issue. Please file it here: https://github.com/xjamundx/eslint-plugin-standard

@lolmaus

This comment has been minimized.

Copy link

lolmaus commented Aug 10, 2016

@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Aug 10, 2016

@dcousens in your example up there was that about destructuring?
let { x } = y

I'd be pretty happy doing file-wide enforcement. I was the one that wrote that original instance-wide enforcement before and probably should have done file-wide instead.

My personal pref is { x: 1 } and [1, 2] as well.

@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Aug 11, 2016

@xjamundx no, it was about object definitions in general

@feross

This comment has been minimized.

Copy link
Member

feross commented Sep 10, 2016

No spaces (e.g. {x: 1}, let {x} = y, and [1, 2]) is more consistent.

But there's an argument to be made that { x: 1 }, let { x } = y, and [1, 2] make more sense since there's another rule that requires spaces around { } in blocks, e.g. a => { return a + 1 }. But blocks are pretty different from objects, deconstruction, etc.

And like @dcousens says, it makes objects visually distinctive from arrays. This is also currently my preferred code style.

@feross

This comment has been minimized.

Copy link
Member

feross commented Sep 10, 2016

I think that standard should aim for consistency whenever possible, so I'm leaning towards {x: 1}, let {x} = y, and [1, 2].

@jprichardson

This comment has been minimized.

Copy link
Member

jprichardson commented Sep 10, 2016

My preferred style is { x: 1 }, let { x } = y, and [1, 2] as well. Since we have spaces for blocks and spaces everywhere else other than [1, 2]... maybe it makes sense to [ 1, 2 ]? Gah, but then that seems weird with someFunc(a, b) and not someFunc( a, b ). There's no easy answer here.

@feross

This comment has been minimized.

Copy link
Member

feross commented Sep 10, 2016

@jprichardson Good points. Seems that no spaces (except for blocks, which are pretty different) is the most consistent, no?

@thejohnfreeman

This comment has been minimized.

Copy link

thejohnfreeman commented Jan 21, 2017

I see this issue hasn't had any discussion in almost 4 months. Regarding @feross's comment, "We can revisit all this in early 2017", maybe now is the time.

After reading all the discussion, my preference has converted from spaceless-objects to spacey-objects, but now I also want spacey-arrays for a few reasons already mentioned:

  1. Consistency with spacey-objects.
  2. Fits the tradition and convention of standard. @zaynv said "I always associated standard with being very 'spacey'."
  3. Consistency with arrays split over multiple lines. @dcousens said:
var u = {
  x: 1,
  y: 3
}
var f = {x: 1, y: 3}

The declarations of f and u in the first case also seem inconsistent... somehow.

Well, we can do similar with arrays:

var a = [
  x,
  y
]
var b = [x, y]

Are these similarly inconsistent? Probably. This argument isn't that convincing to me, but it goes with the other two.

@jokeyrhyme

This comment has been minimized.

Copy link
Contributor

jokeyrhyme commented Jan 21, 2017

It'd be handy for some accord to be reached between https://github.com/jlongster/prettier and JavaScript Standard

@mightyiam

This comment has been minimized.

Copy link
Contributor Author

mightyiam commented Jan 22, 2017

@jokeyrhyme I would discuss that in another issue.


I'm 👍 on spacey. And on determining this.

@saadq

This comment has been minimized.

Copy link

saadq commented Jan 22, 2017

I'm also on the spacey side. Arrays and object literals are two different things, so I think the "inconsistency" between [x, y] and { x, y } makes sense.

@tunnckoCore

This comment has been minimized.

Copy link

tunnckoCore commented Mar 2, 2017

What I was thinking about last few months. I'm pretty used to spacey objects, but not spacey arrays (maybe sometimes, hm).

var a = { x: 2 }
var a = {
  x: 1
}
var a = {
  x: 1,
  y: 2
}
var a = [
  x,
  y
]
var b = [x]
var b = [123]
var b = [ x, y ]
var b = [ { a: 'b' }, 2, { c: 3 }, 4 ]
var b = [{ a: 'b' }]
var b = [ { a: 'b' } ] // too much spaces? above is better
var b = {
  a: 1,
  b: {
    c: [ { cc: 22 }, 333 ]
  },
  d: [ 'e', 444, [ 1, 2, 3 ] ]
}

If we apply consistency, it may seem too spacey in some times, but in most cases it would be separated in separate lines, so won't maybe won't look too spacey. As about the array-bracket-spacing i think we should use singleValue: false option

@cayuu

This comment has been minimized.

Copy link

cayuu commented Mar 2, 2017

It's worth noting that this change (no matter which way we go - but especially spaced-arrays), is likely to affect a significant portion of standard users.

We have three options for concluding this:

  1. Defer to @feross for a decision
  2. Use the most voted/weighted preference of the 16 current participants
  3. Defer to conventions used in popular of ES6+ projects (perhaps looking at the OSS projects from the big companies listed in the README)

My preference would be for 1 or 3. I'm wary of a "happened to be in the room" policy approach (2).

@dcousens

This comment has been minimized.

Copy link
Member

dcousens commented Mar 2, 2017

What is the ecosystem impact of either?

@cayuu

This comment has been minimized.

Copy link

cayuu commented Mar 2, 2017

Started at the top of the README list of companies, left to right:

  • npm: spaced objects, tight arrays
  • opbeat: spaced objects, tight arrays

..then I got tired looking ;)

@jokeyrhyme

This comment has been minimized.

Copy link
Contributor

jokeyrhyme commented Mar 2, 2017

For some perspective, it looks like the prettier folks couldn't decide and just made this a "bracketSpacing" option:

https://github.com/prettier/prettier#api

@wuhkuh

This comment has been minimized.

Copy link

wuhkuh commented Mar 2, 2017

@jokeyrhyme
Doesn't that defeat the purpose of a standard?
The decisions should be up to this package.

@jokeyrhyme

This comment has been minimized.

Copy link
Contributor

jokeyrhyme commented Mar 2, 2017

@wuhkuh I just mention prettier, because it's another example of an opinionated style guide, and it's been a difficult enough decision to make that they've made it an option

In principle, I agree that eliminating pointless bike-shedding and decision-making is the noble goal of an opinionated project

@wuhkuh

This comment has been minimized.

Copy link

wuhkuh commented Mar 2, 2017

Despite the fact that npm and opbeat use tight arrays, I still prefer spacey for the reasons @thejohnfreeman put down.
It would feel unnatural if you keep the arrays tight while you space out objects and functions with their arguments. When in doubt, space out.

There are also tools available that update existing code to suit the standard. This debunks 'patching my enormous package is a ton of work!'.

I would like to add that if we decide which way to go, we could start by sending warnings instead of erroring when this patch is just introduced.
This would decrease the shock of this big decision at the package maintainers that use this standard.

Edit

To counter my bias towards spacey, consider the following situation:

const obj = { ooh: 'that', is: 'spacey' }
console.log(obj[ 'is' ]) // wait, what?
@xjamundx

This comment has been minimized.

Copy link
Contributor

xjamundx commented Mar 2, 2017

paypal uses tight arrays FWIW

@LinusU

This comment has been minimized.

Copy link
Member

LinusU commented Mar 2, 2017

Just realized that having spacey arrays is a way to distinguish array literals and property access 🤔

const array = [ 1 ]
const number = array[0]
@tunnckoCore

This comment has been minimized.

Copy link

tunnckoCore commented Mar 2, 2017

@LinusU good point. :)

@wuhkuh console.log(obj[ 'is' ]) is another rule that i think it should enforce non-spacey style - currently is either set by eslint-plugin-standard.

@falmar

This comment has been minimized.

Copy link

falmar commented Mar 17, 2017

I see more often about spacey objects than spacey arrays, space arrays feel a bit awkward to me

const obj = { a: 1, b: 2 }
const arr = [1, 2]

I personally find this more readable

import React, { Component } from 'react'
import { connect } from 'react-redux'

import { getItems } from '../actions'

const objItems = [{ one: 1 }, { two: 2 }]

const App = () => (
  <div
    style={{ height: '100vh' }}
    objItems={objItems}
    arrItems={[1, 2]}
  />
)

const mapStateToProps = ({ aReducer, bReducer }) => ({ aReducer, bReducer })

export default connect(mapStateToProps, { getItems })(App)

Than this

import React, {Component} from 'react'
import {connect} from 'react-redux'

import {getItems} from '../actions'

const objItems = [{one: 1}, {two: 2}]

const App = () => (
  <div
    style={{height: '100vh'}}
    objItems={objItems}
    arrItems={[1, 2]}
  />
)

const mapStateToProps = ({aReducer, bReducer}) => ({aReducer, bReducer})

export default connect(mapStateToProps, {getItems})(App)

PD: Sorry for using long noisy examples

@juliaogris

This comment has been minimized.

Copy link

juliaogris commented Apr 4, 2017

For those of you looking for a quick fix in the meantime:

  • npm install --global eslint eslint-config-standard eslint-config-standard-jsx eslint-plugin-promise eslint-plugin-react eslint-plugin-standard
  • echo '{"extends": ["standard", "standard-jsx"], "rules": {"object-curly-spacing": [1, "always"]}}' > ~/.eslintrc

Careful, this will replace an exisiting ~/.eslintrc .
Replace "always" with "never" if preferred or read up on the ESLint rule object-curly-spacing and array-bracket-spacing.
Then use with:

This workaround got rid of the majority of my non-fixable standard.js errors. Apparently, at least for me, inconsistent spacing happens a lot less often with array-bracket-spacing. So my suggestion is to just define object-curly-spacing for the time being and leave array spacing as is (consistent).

Count me in on spacey objects.

❤️ Standard.js

@wuhkuh

This comment has been minimized.

Copy link

wuhkuh commented Apr 4, 2017

I might have changed my mind on this. An array looks clean already without spaced-style, and spaced arrays make statements and declarations harder to read in some cases.

const Obj = {
  properties: [ { type: Object } ] // this sucks. Lets add an exception
}
const Arr = [ ] // sure
console.log(Obj[ 'properties' ]) // this sucks. Lets add an exception

Before you know it, we forgot what we tried to achieve: consistency. If we want to patch those situations we would need exceptions to keep it readable (and convenient), therefore making it less consistent.
For anyone in doubt: enforce spaced arrays for a while. You won't quite enjoy them as much as you'd think. I've done this and it changed my mind completely.

My vote is for spaced objects and tight arrays. Arrays are readable and consistent as they are.
I think the most of us agree on spaced objects as far as this thread goes.

@feross Could you update us on the status?

@tunnckoCore

This comment has been minimized.

Copy link

tunnckoCore commented Apr 4, 2017

console.log(Obj[ 'properties' ]) // this sucks. Lets add an exception

That's totally different rule - computed-property-spacing. And is is set to either by eslint-plugin-standard which means it is allowed, but yea it really sucks that's why I force it to ["error", "never"]

edit:
As about

const Arr = [ ] // sure

I really dont like empty things to have space [ ] and { }, it's ugly and needless to me.

@mightyiam

This comment has been minimized.

Copy link
Contributor Author

mightyiam commented Apr 5, 2017

I was just mentoring a person. On a project where we use standard. And I chose to just not tell him to space out the objects. It came out like this: {foo: { bar: 'bar' }}. So the outer object is tight and the inner is spaced. But I didn't tell him to change it. Because we have a linter. And I am not it. Can we go the route of spaced objects and get on with it?

@rstacruz

This comment has been minimized.

Copy link
Member

rstacruz commented Apr 5, 2017

I like this a lot. I'm sure it will impact a lot, so maybe for the next major version.

@alejandrodnm

This comment has been minimized.

Copy link

alejandrodnm commented Apr 6, 2017

Hey guys it's been almost a year since this issue was opened. I'm going to add a quote from the FAQ

At the end of the day you have to 'just pick something', and that's the whole philosophy of standard

I think in all this time people have been given the opportunity to express their opinion, but it's time to decide.

I really don't have a preference, I just want to see this issue close, I have colleges using both approach and I can't enforce a style on them, because as @mightyiam just said, we have a linter for that (That we really like) and the CI passes.

You are always going to find people who prefer one way or the other, but I don't think they will just remove standard from their projects just because they disagree with a rule.

Cheers.

@feross

This comment has been minimized.

Copy link
Member

feross commented Apr 6, 2017

Rest assured that this will be decided in a future version. There's no need for folks to continue weighing in.

The past few releases have had some pretty large breaking changes, and there's a limit to how many breaking changes we can include in a given release.

I'll lock this issue to save everyone some time.

@standard standard locked and limited conversation to collaborators Apr 6, 2017

@standard standard unlocked this conversation Aug 28, 2018

@feross

This comment has been minimized.

Copy link
Member

feross commented Aug 28, 2018

Thank you to everyone for sharing your thoughts on this issue. Thanks to @LinusU for the analysis in this comment and for sending a PR: standard/standard#609 (comment)

The rule to enforce spacing inside of braces will be included in the standard 12 release which will happen later tonight. Happy to merge this and put this one behind us! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.