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

Clarification of bracketSpacing option #715

Closed
benjaminapetersen opened this Issue Feb 15, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@benjaminapetersen
Copy link

benjaminapetersen commented Feb 15, 2017

I'm not 100% sure what the bracketSpacing config does.

These explanations:

  • // Controls the printing of spaces inside object literals from the README.md
  • // Controls the printing of spaces inside arrays and objects from the atom package settings page

When using, the following code:

// source:
let list = [{
    name: "foo",
    value: "bar"
  },{
    name: "bar",
    value: "baz"
  }];

The output is always the same, wether bracketSpacing is used or not (cli or atom plugin):

// output:
let list = [
  {
    name: "foo",
    value: "bar"
  },
  {
    name: "bar",
    value: "baz"
  }
];

I'm assuming I'm just expecting something its not designed to do. With bracketSpacing disabled, I would have expected it to stay more dense. Any clarification would be great. Thanks!

@rhengles

This comment has been minimized.

Copy link
Contributor

rhengles commented Feb 15, 2017

That config only makes a difference when the object fits on a single line:

const obj1 = { a: 1, b: 2, c: 3 }; // bracketSpacing: true
const obj1 = {a: 1, b: 2, c: 3}; // bracketSpacing: false
@benjaminapetersen

This comment has been minimized.

Copy link

benjaminapetersen commented Feb 15, 2017

Ahh... ok great. Perhaps is just a README/doc update then. I'd be happy to PR the README change if it would be helpful.

@vjeux

This comment has been minimized.

Copy link
Collaborator

vjeux commented Feb 15, 2017

@benjaminapetersen would be awesome to give examples of what each options look like in the readme!

@vjeux vjeux added the type:docs label Feb 15, 2017

@benjaminapetersen

This comment has been minimized.

Copy link

benjaminapetersen commented Feb 15, 2017

Cool. So I'm curious, there are very few options (implying opinion is good), why even have bracketSpacing as an option at all? Seems like this one & trailingComma (suppress) are just worth enforcing. However, printWidth, tabWidth, and singleQuote seem like basic essential settings.

@vjeux

This comment has been minimized.

Copy link
Collaborator

vjeux commented Feb 15, 2017

Making trailing commas the default seems like a good idea! @jlongster we could use that option for the es7 trailing commas. I don't think that trailing commas are very controversial

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 15, 2017

I'm also curious about why the bracketSpacing options exists at all. printWidth, tabWidth and singleQuote are things I could easily see people starting flame wars over, but bracketSpacing?

@jlongster

This comment has been minimized.

Copy link
Member

jlongster commented Feb 20, 2017

People are pretty opinionated about trailing commas, and I think it's worth having it as an option. However, I am open to removing the bracketSpacing option. I haven't seen many people talk about it or use it. I think it's mainly there because recast had that option.

@vjeux

This comment has been minimized.

Copy link
Collaborator

vjeux commented May 21, 2017

The new docs have it clarified

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