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

Separate concept of brackets/braces #296

Closed
joefiorini opened this issue Jan 18, 2017 · 14 comments
Closed

Separate concept of brackets/braces #296

joefiorini opened this issue Jan 18, 2017 · 14 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken

Comments

@joefiorini
Copy link
Contributor

See example here:

https://jlongster.github.io/prettier/#%7B%22content%22%3A%22const%20aList%20%3D%20%5B1%2C%202%2C%203%5D%3B%5Cnconst%20anObj%20%3D%20%7B%20a%3A%20'b'%2C%20c%3A%20'd'%7D%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3Afalse%2C%22bracketSpacing%22%3Atrue%7D%7D

The default airbnb rules require no spaces directly inside brackets (see https://github.com/airbnb/javascript#whitespace--in-brackets) and a space inside curly braces (see https://github.com/airbnb/javascript#whitespace--in-braces). It would be nice to have separate bracket-spacing and curly-brace-spacing options to allow similar flexibility.

I'm surprised I couldn't find an existing issue for this, as it must be causing eslint conflicts for a lot of projects. Maybe I just missed it?

@vjeux vjeux added the status:needs discussion Issues needing discussion and a decision to be made before action can be taken label Jan 18, 2017
@vjeux
Copy link
Contributor

vjeux commented Jan 18, 2017

I'm surprised I couldn't find an existing issue for this, as it must be causing eslint conflicts for a lot of projects. Maybe I just missed it?

prettier has just been open sourced a week ago and in the past week we've fixed a bunch of things that made it output invalid JavaScript and code that looks really bad. So I don't expect that many people actually managed to actually use it for their project yet.

Please keep opening issues, they are great!

@joefiorini
Copy link
Contributor Author

@vjeux thanks, I didn't realize this repo was still so young. I am using it in my work codebase right now, so I will keep opening issues as I come across things that conflict with our (fairly standard airbnb) eslint config.

@vjeux
Copy link
Contributor

vjeux commented Jan 18, 2017

At this point, the biggest thing that is broken are comments. You can go through the list: https://github.com/jlongster/prettier/issues?q=is%3Aissue+is%3Aopen+label%3AComments

  • We output invalid JavaScript // comment else {
  • We drop comments inside of empty blocks
  • Comments are positioned very randomly and may make eslint-ignore-next-line kind of comments invalid.

Outside of that, it should output valid JavaScript, but there are some things we can output better like JSX (#234), Binary expressions (#262) and Member expressions ( https://github.com/jlongster/prettier/issues?q=is%3Aissue+is%3Aopen+label%3AMemberExpression )

The goal is to get through most of them by end of next week :)

@vjeux
Copy link
Contributor

vjeux commented Jan 18, 2017

Our plan of attack is to fix all those things and when it is done, go through a pass of all the suggestions like this one and evaluate what we want to do with them :) Keep them coming!

@joefiorini
Copy link
Contributor Author

I might implement something for myself for the time being, so I can keep my current project in line with our eslint config (if I can do it quickly enough).

@vjeux
Copy link
Contributor

vjeux commented Jan 18, 2017

Should be easy, search for options.bracketSpacing in src/printer.js and remove the line when you don't want it :)

@joefiorini
Copy link
Contributor Author

joefiorini commented Jan 18, 2017

In case someone else needs this change for now:

I went through and changed options.bracketSpacing to options.bracketSpacing || true in every context except array. Also noticed a place where shouldPrintSpaces is set to options.bracketSpacing, but then never used (around src/printer.js:1842). Assuming that isn't intentional?

@vjeux
Copy link
Contributor

vjeux commented Jan 18, 2017

Nope, not intentional, feel free to send a pull request to get rid of it :)

@jlongster
Copy link
Member

This is an opinionated formatter so we're not going to keep adding options so that people can still maintain their own formatting rules. I don't recommend using this with eslint's formatting rules. You should get rid of the formatting rules and keep the other rules that enforce semantic behaviors.

I know that's not always possible for existing projects, and we're trying to find a balance for adding a few options to help adoption, but we're also not going to add a lot of configuration.

@jlongster
Copy link
Member

BTW, I'm open to never adding spaces with square brackets if that's how most people want it, disregarding the bracket spacing option.

@smably
Copy link
Contributor

smably commented Jan 24, 2017

I did a survey of the most popular JS coding styles. Here is what I found:

None of these style guides recommend spaces inside brackets, nor do the examples in Crockford or Mozilla's code style pages. Standard's examples also omit the spaces inside brackets.

My recommendation: prettier shouldn't insert spaces inside brackets, but spaces inside braces should remain configurable.

vjeux added a commit to vjeux/prettier that referenced this issue Jan 24, 2017
…enabled

Given the discussion on prettier#296, it seems like there's debate between spaces around `{}` but no one puts spaces around `[]`. So changing the behavior to respect this.
@vjeux
Copy link
Contributor

vjeux commented Jan 24, 2017

I created #446 to remove spacing inside of []

vjeux added a commit to vjeux/prettier that referenced this issue Jan 24, 2017
…enabled

Given the discussion on prettier#296, it seems like there's debate between spaces around `{}` but no one puts spaces around `[]`. So changing the behavior to respect this.
vjeux added a commit that referenced this issue Jan 24, 2017
…enabled (#446)

Given the discussion on #296, it seems like there's debate between spaces around `{}` but no one puts spaces around `[]`. So changing the behavior to respect this.
@delijah
Copy link

delijah commented Jan 31, 2018

If this library is about enabling developers to use their own code style, why do you define fixed rules here? I think both should be configurable, spaces inside curly braces and spaces inside (square) brackets (separately).

@lydell
Copy link
Member

lydell commented Jan 31, 2018

If this library is about enabling developers to use their own code style

It is not.

https://prettier.io/

Opinionated Code Formatter

https://prettier.io/docs/en/why-prettier.html

By far the biggest reason for adopting Prettier is to stop all the on-going debates over styles.

Less configuration, less debate.

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:needs discussion Issues needing discussion and a decision to be made before action can be taken
Projects
None yet
Development

No branches or pull requests

6 participants