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

Option for (no) semi #900

Closed
wants to merge 3 commits into from
Closed

Option for (no) semi #900

wants to merge 3 commits into from

Conversation

pre63
Copy link

@pre63 pre63 commented Mar 6, 2017

I implemented a prototype for a option to remove the semi. It removes all the semi if semi=true. It's a brute force approach and does not account for the cases where not having a semi is a breaking change.

I'd be ready to add documentation and test if you are interested in merging this pr.

Then the next step would be adding specific logic for the cases where an absent semi is breaking. But it's more a v2 thing.

@vjeux
Copy link
Contributor

vjeux commented Mar 6, 2017

Thank you for this! We do want to allow an option to print code without a semi-colon. However, we cannot ship this one as is as it would output code that doesn't have the same semantic.

For example

console.log();
[1, 2, 3].forEach(a => a);

would be transformed into

console.log()
[1, 2, 3].forEach(a => a)

which throws

Uncaught TypeError: Cannot read property '3' of undefined

as it is evaluated as

console.log()[1, 2, 3].forEach(a => a)

If you are willing to make an implementation that produces correct code, I'd love to merge it (but beware, it's not easy!)

@pre63
Copy link
Author

pre63 commented Mar 6, 2017

@vjeux I agree it's a problem. In your opinion what would be the best outcome for this code?

console.log();
[1, 2, 3].forEach(a => a)

or

console.log()
;[1, 2, 3].forEach(a => a)

@vjeux
Copy link
Contributor

vjeux commented Mar 6, 2017

I've never written code without semi-colons so I'm not sure. Do you know if there's a conventional way? Good places to look would be the standard project and what the autofix rule of eslint does.

@pre63
Copy link
Author

pre63 commented Mar 6, 2017

There are 3 main problems that I know of.

  1. The one you described
console.log()
[1, 2, 3].forEach(a => a)

evaluated to

console.log()[1, 2, 3].forEach(a => a)
  1. the self calling function
console.log('hello')
(function(){
  console.log('world') // throws
}())
  1. the return undefined statement
return
  { hello:world' }

becomes

return;
  { hello:world' };

I'll give it a try.

I'll add the semi at the beginning of the line because I think it's easier and because it's part of the standard. Eslint rule can always be adapted.

@vjeux
Copy link
Contributor

vjeux commented Mar 6, 2017

There are many more, some examples:

for (;;);
console.log()

#634 (comment)

class A {
  a = 1;
  [b] = 2;
}

#12 (comment)

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

@lydell
Copy link
Member

lydell commented Mar 6, 2017

As far as I know, leading semi-colon is the convention.

console.log()
;[1, 2, 3].forEach(a => a)

The idea is to clearly show that a semi-colon is actually required there. If you put it after console.log() it might look like a mistake. Also, if the console.log() is moved, you'll have to remember to move the semi-colon appropriately.

@pre63
Copy link
Author

pre63 commented Mar 6, 2017

@lydell I agree

@jlongster
Copy link
Member

@pre63 Thank your for this PR. This is a feature that I was planning to work on and I would like to take it because it requires a lot of special work and also forces us to figure out how to run tests with it and without, and stuff like that. It's such a large feature that I think it's going to be easiest if I implement it. Is that OK? We're also not 100% committed to this feature yet; we want to explore the complexity of it and see if it's simple enough. If it's too buggy/complex we're not going to do it.

Sorry for the work you already put into it. Almost all other bugs/features are open for the taking but this one is different. Is it OK if we close this while I work on another PR?

@vjeux
Copy link
Contributor

vjeux commented Mar 7, 2017

I'm going to close it in order to make the queue smaller as @jlongster mentioned.

@vjeux vjeux closed this Mar 7, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants