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

Add option to omit semicolons in output #12

Closed
iki opened this issue Jan 10, 2017 · 43 comments
Closed

Add option to omit semicolons in output #12

iki opened this issue Jan 10, 2017 · 43 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!

Comments

@iki
Copy link

iki commented Jan 10, 2017

E.g. semicolons: true to use semicolons at the end of statements (default), and semicolons: false to not use semicolons.

@rogeliog
Copy link
Contributor

@jlongster If you are both accepting PRs and want this feature, I would be more than happy to help with a PR for this one.

@jlongster
Copy link
Member

jlongster commented Jan 10, 2017

I'm open to it, but it gets problematic fast. When you don't use semicolons, you run into things like this where you are forced to put a semicolon:

var x = 5

;[1, 2, 3].forEach(...)

The first time this would be parsed correctly, but we would output:

var x = 5

[1, 2, 3].forEach(...)

which is invalid JS. (at least, not what you want.) This means that in order to support semicolon-less JS we need to code in all the ASI rules so we can see where semicolons are important. I'm not sure I'm willing to take on that complexity just yet.

Feel free to play around with it. Code is powerful and you might convince me. My preference for evolving projects is to take it slowly though, and it's hard to remove things later than to add them, so let's play with it over time and see what we can do. I'd like to keep the number of options low, but this is such a divisive issue that it could be worthwhile.

@gaelollivier
Copy link

Have two suggestions:

  • what about making this the default (and only) behavior ? I agree configuration options should be avoided so why not considering making this the standard ?
  • how about auto-detecting depending on the existing code ? If it has semi-colons, output semi-colons, if not, do not output them ?

@swansontec
Copy link

swansontec commented Jan 10, 2017

I use standard.js, which is a popular no-configuration style guide & linter. They insist on a semicolon-free style, and the only rule needed to make this work is to never start a line with (, [, or `.

I would love to see standard.js and prettier be compatible one day.

@K-Leon
Copy link

K-Leon commented Jan 10, 2017

I would love seeing this integrated - is currently the only show stopper for me.

// Edit: Standard Packe had 200k+ Downloads last Month - i think it's something that matters for a lot of people in the JS Community

@bakkot
Copy link
Collaborator

bakkot commented Jan 11, 2017

It's worth mentioning that "the only rule needed to make this work" will get progressively more complex as the language evolves. For example, under the popular public fields proposals these two classes differ:

class A {
  set
  foo(x){} // setter for 'foo'
}

class B {
  set; // uninitialized field named 'set'
  foo(x){} // method named 'foo'
}

(Note that babylon currently gets these edge cases wrong in a number of exciting ways.)

Presumably a no-semi style would have you writing

class B {
  set
  ;foo(x){}
}

So, keep in mind that the complexity cost comes not just now but also in the future. (I don't think this is well understood by people using this style.)

@tunnckoCore
Copy link

tunnckoCore commented Jan 11, 2017

Huh. I found cool thing. Because I'm moving to Atom and just using ESLint instead of Standard, i just love the combo of Linter, Linter-ESLint + eslintrc, and atom prettier :D

It is just a whole show, because I had semi: [2, always], but atom linter can autofix (on save) the fixable errors, so I'm still no writing semicolons. Between that phase prettier prittifies and changes single quotes to double quotes (eslint warns me for a milliseconds there is doublequotes) and Atom Linter (because eslintrc) converts them back to singlequotes and insert the semicolons.

There has a bit of delay between them and it can be seen what does what.

Cool. I'm moving back to semicolons because the AtomLinter.
As I know SublimeLinter don't have support for autofix.

edit: So in short. I also wanted that option, but it seems AtomLinter comes to the rescue and can be easily fixed if you use ESLint, not some "standards" or "semistandards" :)

@MoOx
Copy link
Contributor

MoOx commented Jan 11, 2017

Just have a simple question.
If people write

var x = "thing"

[1].forEach(...)

how do you know the person wanted to write

var x = "thing";

[1].forEach(...);

and not

var x = "thing"[1].forEach(...)

My point is "how do we know?" because if we know, we can add or prepend a semicolon, so same thing right?

@dtinth
Copy link
Contributor

dtinth commented Jan 11, 2017

I tried piping the result of prettier into standard --fix and the result is quite promising: prettier-standard-formatter.

// Before
var x = 5;
[1,2,3].forEach(console.log);

// After
var x = 5;
[ 1, 2, 3 ].forEach(console.log)
// Before
var x = "thing"
[1].forEach(console.log)

// After
var x = 'thing'[1].forEach(console.log)

If the user doesn’t include a semicolon in front, these two lines gets squashed into the same line, clearing any ambiguity.

@wmertens
Copy link

@MoOx There is only one way that your example will be parsed (so one canonical AST), and only two ways that prettier should output it:

with semi:

var x = "thing";

[1].forEach(...);

without semi:

var x = "thing"

;[1].forEach(...)

The rule is simply: remove semi at end of line, and insert semi at beginning of line, before array literal, parens block, or template string.

@MoOx
Copy link
Contributor

MoOx commented Jan 11, 2017

There is only one way that your example will be parsed (so one canonical AST), and only two ways that prettier should output it

I know that. My idea is "what if people write this by mistake". I guess the response is "pebkac".

@wmertens
Copy link

wmertens commented Jan 11, 2017

@bakkot Yes, indeed, in this case the semicolon injection needs to happen after the keywords set and get in classes if used as class field names. Indeed, this is extra maintenance. But is it a lot of maintenance? Will there be a lot of cases like this? Personally I'm skeptical.

@timdorr
Copy link

timdorr commented Jan 11, 2017

@MoOx

how do you know the person wanted to write

Seems as simple as looking that there's a linebreak/whitespace there and assuming they intended to have those two tokens be separate. Basically:

"thing" [1] // implicit semicolon
"thing"
[1] // implicit semicolon
"thing"[1] // no implicit semicolon

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

dtinth commented Jan 11, 2017

@timdorr

Seems as simple as looking that there's a linebreak/whitespace there

However, that goes against the rules of JavaScript. A JavaScript engine will parse it as "thing"[1] no matter what. I don’t think a code formatter should treat "thing" [1] as "thing";[1] which alters the meaning of the code.

@MoOx

what if people write this by mistake

The no-unexpected-multiline ESLint rule will warn people about this 😃.

P.S. I think the implications of using a semicolon-free style has already been well studied and there are already linter rules to avoid making mistakes in both styles. I would suggest to focus the discussion more on the complexity incurred and implementation effort required.

@bakkot
Copy link
Collaborator

bakkot commented Jan 11, 2017

@wmertens Not just 'get' and 'set' but also 'static' - but not 'async'.

Off the top of my head, there's also

class A {
  a = 0; // initialized class field
  *b(){} // generator method
}

class B {
  a = 0
  *b(){} // SyntaxError
}

And that's just with this one proposal.

It's hard to predict how much more complex it'll be in the future. That's kind of my point: adding this option will add an unknown (but certainly nonzero) amount of future complexity.

@TehShrike
Copy link

Why not just leave semicolons alone, rather than adding or removing?

@dtinth
Copy link
Contributor

dtinth commented Jan 11, 2017

@TehShrike Because this tool doesn’t care about the existing formatting of the code.

From the README

In technical terms: prettier parses your JavaScript into an AST and pretty-prints the AST, completely ignoring any of the original formatting. Say hello to completely consistent syntax!

It’s more complex to program the pretty-printer not to output a semicolon, than to make it always output one.

@jlongster
Copy link
Member

Seems as simple as looking that there's a linebreak/whitespace there and assuming they intended to have those two tokens be separate. Basically:

Yes, and this is what I meant by "we'll have to start embedding ASI-style rules into the printer".

@kentcdodds
Copy link
Member

kentcdodds commented Jan 11, 2017

Any chance we could start small with what @TehShrike suggested?

Why not just leave semicolons alone, rather than adding or removing?

As an option. So: addSemicolons which would default to true

Actually, this would be enough for me, as eslint --fix will remove semicolons for me anyway. So if prettier misses removing a semicolon, eslint will do it. So if I had the ability to prevent prettier from adding semicolons then I'd be good :)

@TehShrike
Copy link

@dtinth but semicolons are code, too. I would expect them to be represented in the AST.

@jlongster
Copy link
Member

Why not just leave semicolons alone, rather than adding or removing?

The AST has no notion of "semicolons" at all. And this formatter works by printing the AST, with a few exceptions where we look at the original text. Those few exceptions are very easy cases, while checking for semicolons would be far harder.

Any chance we could start small with what @TehShrike suggested?

It'd require a ton of special-casing and isn't really trivial. This formatter discourages giving the programmer control because the goal is complete consistency. The one exception is blank lines -- we will keep blank lines because that's such an important part of the code (but we will still collapse multiple blank lines into a single one).

@kentcdodds
Copy link
Member

Makes sense.

giphy

@jlongster
Copy link
Member

standard is a widely excepted format for semicolon-less JavaScript. Since there's a viable option for semicolon-less formatting already, I think this project should focus on a format that includes them (and there are technical reasons for this too, it would add a bunch of complexity).

If you don't want semicolons, use standard. Or as someone else mentioned, fork this project and try to implement it yourself. I'll take a look and we can talk more then. standard has a few forks as well that tweak basic things like semicolons (semi-standard, etc), and I think it's fine if there's a fork that has that on by default. That would fix the testing problem, too -- we use snapshots, so how would we make sure both semicolons and no-semicolons works when PRs come in? Would we have to have two identical test suites that have snapshots with different options? In fact, we have other options as well, so should we have 4-5 identical test suites with different snapshots?

I think a fork is actually a viable option and helps relieve the tension between the different styles. I'm happy to work on whatever tooling/process is needed to help maintain a fork if people really want that style.

Lastly, we can also make it easy to integrate prettier in other workflow. For example this kind of works:

prettier foo.js | standard --stdin --fix

Although if you're really after just ditching semicolons, you probably just want eslint directly that removes semicolons. But that might be kind of slow. If all you want is dropping the semicolons, I wonder if you could do it as a quick post-processing step or something. I think this kind of thing can be explored outside of prettier for now.

If someone wants to explore a no-semicolons version of this, I'd love to see it. I'm happy to advocate a forked version of this that has that, or a post-processing step, or something like that.

@jlongster
Copy link
Member

Since we won't be actively working on this, I'm closing this issue. But whoever reads this please know: the next step is to implement it yourself and show me the code. We can see how much complexity it adds and talk about if a fork is the right option or not.

@dtinth
Copy link
Contributor

dtinth commented Jan 12, 2017

Here’s my take for standard users: prettier-standard-formatter.

It basically takes the output from prettier and passes it into standard --fix.

An Atom package is available.

@kentcdodds
Copy link
Member

@tunnckoCore, thanks for the tip! I'd added it to my devDependencies and yarn doesn't move the dependency when you do yarn add (I should probably file a bug)... In any case, it's been fixed. Thank you.

Why not just use prettier plugin and linter-eslint it works great as I already mentioned.

Because this:

working

So instead, I can disable --fix on linter-eslint and just use prettier-eslint-atom (as soon as it gets a few kinks worked out) and I don't get that flash of formatting. I'd say it's worth the effort.

The other underlying idea behind something like prettier-eslint is as I said elsewhere:

It's more about being able to take advantage of the advanced formatting capabilities of prettier, without sacrificing the stylistic choices of people already using ESlint to autofix/format their code. Whether that be semicolons or spacing in parameter lists.

If you don't want to use it, be my guest 😄

@tunnckoCore
Copy link

Hm. I'm new to Atom. Yea I did notice that flash and it wasn't cool, and kinda feel slower that it should. I did follow the linked issue.

So yea, maybe make sense and I'll try your plugin.

In anyway, that specific gif seems more like a prettier bug to me. Why moves the inline comment on new line? Hm.

@kentcdodds
Copy link
Member

I'm pretty sure that prettier disallows comments on the same line (which is cool with me), but it'd make more sense for it to go above. In any case, discussion about that should go in a new issue. What I was trying to point out in the gif is the flash of semicolons (because prettier) and then their subsequent removal (because eslint --fix). Whereas with my plugin, things are much smoother:

plugin

Note again that the plugin's got some issues that need PRs :)

@tunnckoCore
Copy link

In any case, discussion about that should go in a new issue.

Yea, yea, I'm agree.

@jlongster
Copy link
Member

(We don't disallow comments on the same line, inline comments are just plain buggy right now)

@rhengles
Copy link
Contributor

Has anyone here already made a fork with a configuration option to remove semicolons? If anyone did it or will do it, I'll try to merge it in my fork. Or if someone sends a PR, even better! 😃

@tunnckoCore
Copy link

Omg. "with tabs". I can't understand why complexing the tools (not talking only specifically for prettier) when there always are workarounds with other tools and existing workflow? But yea, our different styles in that community kills us.

@rhengles
Copy link
Contributor

@tunnckoCore

For a few reasons:

  • It's a simple change
  • It's more efficient
  • I personally believe that tabs are the correct way to indent code
  • I won't compromise for a workaround. I want it as a first-class option.

I know I can't change what the majority of the community uses, but I cannot and will not adapt into it simply because it is what the majority uses, but only if someone manages to convince me it is a technically better option.

@sheerun
Copy link
Contributor

sheerun commented Mar 16, 2017

I'm pasting the link to my package I've created specifically to fix this issue, as this thread is the first on google when you search for "prettier semicolons": https://github.com/sheerun/prettier-standard

I hope prettier will support no-semi rule soon enough, though!

@MadelineRitchie
Copy link

For anyone finding this via Google, note that prettier supports this option now, per this PR:
#1129

@alexgvozden
Copy link

eslint.autoFixOnSave is a great option to autofix your code

@j-f1
Copy link
Member

j-f1 commented Jan 12, 2018

@vjeux Should this be unlocked? The option has been added.

@vjeux vjeux reopened this Jan 12, 2018
@prettier prettier unlocked this conversation Jan 12, 2018
@j-f1
Copy link
Member

j-f1 commented Jan 12, 2018

This has been added in #1129.

@j-f1 j-f1 closed this as completed Jan 12, 2018
@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
@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!
Projects
None yet
Development

No branches or pull requests