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

Semicolon option #736

Closed
redstrike opened this issue Feb 19, 2017 · 24 comments
Closed

Semicolon option #736

redstrike opened this issue Feb 19, 2017 · 24 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.

Comments

@redstrike
Copy link

redstrike commented Feb 19, 2017

Many modern languages don't force us to type "semicolon / ;" at the end of each line.
I was so happy to know that JavaScript also supports that feature.
After switched to no semicolon style, I don't want to go back anymore.
Of course, there is some special case that we need a semicolon.
Luckily, VS Code is smart enough to warn me those edge cases.

Therefore, I believe a sophisticated prettier should have an option for "no semicolon-ers".
It should "off / disabled" by default. (I know everyone still loves semicolons)

@rtsao
Copy link
Contributor

rtsao commented Feb 19, 2017

See: #12

You can use https://github.com/kentcdodds/prettier-eslint to remove semicolons via eslint --fix

@redstrike
Copy link
Author

redstrike commented Feb 19, 2017

Oh, I try to search for semicolon issues but forgot to include the closed issues 😶
However, I don't think it's a good idea to install another tool (eslint) for such a simple use case.

No semicolon should be the first class option of prettier. As a user, I want to make the code more pretty to look at, so I decide to use the tool. But, the tool should not mess up with my code (at least, it should not add more characters).

@vjeux
Copy link
Contributor

vjeux commented Feb 19, 2017

This is something we are considering implementing as a lot of people are not adding semi-colons. We need to figure out how it affects correctness, there are places where semi-colons are needed to ensure that the code runs the same way as before.

@redstrike
Copy link
Author

redstrike commented Feb 19, 2017

Ensure the 100% correctness for this use case is difficult (I guess), because even the no-semicolon-ers may not be sure when they have to add edge cases' semicolons. I don't remember well the edge cases, so I am relying on the intelligence of VS Code and TypeScript to highlight them for me.

I believe prettier could prettify the code without adding extra semicolons. VS Code's built-in "Format Document" feature can do it well (I 👍 it), but it lacks useful config options. Also, prettier-vscode seems override the built-in formatting feature.

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

A good basis for implementing such a feature are the exceptions listed in http://eslint.org/docs/rules/no-unexpected-multiline.

@bakkot
Copy link
Collaborator

bakkot commented Feb 20, 2017

@yamafaktory although mind that there are further exceptions in experimental features like class fields.

If y'all do decide to implement this, feel free to ping me to check the logic for class bodies in particular; I took some care getting it right in an experimental implementation in V8.

@sheerun
Copy link
Contributor

sheerun commented Mar 14, 2017

I've created prettier-standard that by default doesn't use semicolon. for now it uses eslint for post-processing but I hope prettier will support it natively

@redstrike
Copy link
Author

redstrike commented Mar 15, 2017

@sheerun Thank you for your efforts! I didn't know that standardjs allows no-semicolon rule 😮 I decided to ignore all standard styles due to their loves to semicolons (while I hate it to the bone).

@rattrayalex
Copy link
Collaborator

In case folks missed it, in his talk at React Conf the other day, @jlongster announced that prettier will be providing this option.

@yukulele
Copy link

yukulele commented Mar 19, 2017

Standardjs give the (rare) cases you must use semicolons:
http://standardjs.com/rules.html#semicolons

@rattrayalex
Copy link
Collaborator

Hmm, surprisingly that's not quite comprehensive. There's also / (division or regex), + and - (addition/subtraction or positive/negative number).

Shameless plug: I recently built LightScript, a JS dialect which, among other things, really never needs semicolons. It's in the early stages and I'd love feedback.

@vjeux
Copy link
Contributor

vjeux commented Apr 5, 2017

Status update on this one, we agreed that we want a --no-semi option and are waiting for someone to implement it. Note that this is not as simple as removing all the existing ;, you need to make sure that it produces a program that has the same behavior as before.

@bakkot
Copy link
Collaborator

bakkot commented Apr 5, 2017

@vjeux Is the plan that it should include exactly the necessary semicolons, or follow some other rule?

e.g., which of the following semicolons would get included and where would they be put? (In other words, how would a no-semi option style this?)

;
(function(){})()
;
(function(){})()
;
if (foo) {}
;
(function(){})()
;

class A {
  get
  ;
  x(){}

  gett
  ;
  x(){}

  a = 0
  ;
  [b] = 1
  ;
  c = 2
  ;
  *d(){}
}

@vjeux
Copy link
Contributor

vjeux commented Apr 5, 2017

@bakkot the consensus among people writing code without semi-colon is that the semi-colon should be printed at the beginning of the next line when required for correctness. Your example would look like:

(function(){})()
;(function(){})()
if (foo) {}
(function(){})()

class A {
  get
  x(){}

  gett
  x(){}

  a = 0
  ;[b] = 1
  c = 2
  ;*d(){}
}

https://prettier.github.io/prettier/#%7B%22content%22%3A%22(function()%7B%7D)()%5Cn%3B(function()%7B%7D)()%5Cnif%20(foo)%20%7B%7D%5Cn(function()%7B%7D)()%5Cn%5Cnclass%20A%20%7B%5Cn%20%20get%5Cn%20%20x()%7B%7D%5Cn%5Cn%20%20gett%5Cn%20%20x()%7B%7D%5Cn%5Cn%20%20a%20%3D%200%5Cn%20%20%3B%5Bb%5D%20%3D%201%5Cn%20%20c%20%3D%202%5Cn%20%20%3B*d()%7B%7D%5Cn%7D%22%2C%22options%22%3A%7B%22printWidth%22%3A80%2C%22tabWidth%22%3A2%2C%22singleQuote%22%3Afalse%2C%22trailingComma%22%3A%22%22%2C%22bracketSpacing%22%3Atrue%2C%22jsxBracketSameLine%22%3Afalse%2C%22parser%22%3A%22%22%2C%22doc%22%3Afalse%7D%7D

@bakkot
Copy link
Collaborator

bakkot commented Apr 5, 2017

@vjeux StandardJS says "Never start a line with (, [, or `", is why I ask, which would imply to me that the first block would be

;(function(){})()
;(function(){})()
if (foo) {}
;(function(){})()

But I don't use the style, so I don't actually know. (Looks like their tooling accepts either.)

Also you dropped a necessary semicolon:

class A {
  get;
  x(){}
}

differs from

class A {
  get
  x(){}
}

@bakkot
Copy link
Collaborator

bakkot commented Apr 5, 2017

@vjeux Already fixed it. Just hasn't made it into the version Prettier is using.

@vjeux
Copy link
Contributor

vjeux commented Apr 5, 2017

Honestly, I don't really care about either way. If it's easier to implement always adding ; before ([{ then it's fine to me. I think it's a pretty edge case and doesn't matter much in practice as long as the result is correct.

Cool for the PR, master is updated to beta7 so it has it fixed now! The website version uses the latest release.

@lydell
Copy link
Member

lydell commented Apr 5, 2017

I’ve worked on several “semi-colon free” projects, and none of them have followed a “use as few semi-colons as possible” rule, but rather a “if a line starts with a ( or [ or whatever, put a semi-colon in front” rule, as @bakkot was hinting at. I guess the biggest reason was because humans can only keep so many edge cases in their heads. That wouldn’t matter when having prettier deal with it for you, but the second biggest reason still holds regardless: It’s easier to move lines and add lines if there are a few extra semicolons.

For example, this:

if (foo) {}
+console.log('test')
(function(){})()

would prettify into:

if (foo) {}
console.log('test')(function(){})()

Then you would have to add in a semicolon manually.

Same thing if you were to move the (function(){})() line somewhere else: You would need to manually add the semicolon (either by remembering it or after seeing how prettier formats it).

So I’d be in favor of prettier formatting it like this:

if (foo) {}
;(function(){})()

In other words: Not trying to print as few semicolons as possible, but always printing a semicolon before “troublesome start-of-line chars”.

@vjeux
Copy link
Contributor

vjeux commented Apr 5, 2017

@lydell thanks for the insights! Sounds like a good plan!

@rattrayalex
Copy link
Collaborator

Well that should make things a lot easier 😄

@rattrayalex rattrayalex mentioned this issue Apr 6, 2017
@rattrayalex
Copy link
Collaborator

Making great progress; hope to have something mergeable by EOD.

@frol
Copy link

frol commented Apr 12, 2017

#1129 is merged! Hooray!

@vjeux vjeux closed this as completed Apr 12, 2017
@yukulele
Copy link

semi option should be added on playground https://prettier.github.io/prettier/

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
@j-f1 j-f1 added status:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! and removed status:needs discussion Issues needing discussion and a decision to be made before action can be taken labels Aug 19, 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:has pr Issues with an accompanying pull request. These issues will probably be fixed soon! type:option request Issues requesting a new option. We generally don’t accept these unless there is technical necessity.
Projects
None yet
Development

No branches or pull requests