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

Do not remove line breaks #105

Closed
mohsen1 opened this issue Sep 26, 2014 · 8 comments
Closed

Do not remove line breaks #105

mohsen1 opened this issue Sep 26, 2014 · 8 comments

Comments

@mohsen1
Copy link

mohsen1 commented Sep 26, 2014

function foo() {
}

function bar() {
}

After fix becomes

function foo() {
}
function bar() {
}

Somehow related to #90

@addyosmani
Copy link
Contributor

+1. I've been running into this too.

@albertjan
Copy link
Contributor

If we want this we need to do a PR for https://github.com/Constellation/escodegen to be able to specify extra newlines above blocks.

See: estools/escodegen#203

@goatslacker
Copy link
Member

I have a branch called recast which uses benjamn/recast instead of escodegen/esprima

If you guys want to run it and let me know what you like/don't like about it @addyosmani @albertjan @mohsen1

There are advantages and disadvantages. Some clear advantages are preservation (for the most part) of formatting and weird spacing, and I believe it has some ES6 support. The disadvantages are that ASI no longer works and neither does JSLINT's white spacing rules.

I've been debating on whether or not to move forward with recast.

@mohsen1
Copy link
Author

mohsen1 commented Sep 29, 2014

I just tried recast. It's cool but it wouldn't give you empty line and comments in the AST. AST by definition shouldn't have that information.

@addyosmani
Copy link
Contributor

@goatslacker Thanks for the heads up about the recast branch. I'll be happy to try it out. Preservation of formatting would be ideal but I'll need to see how finicky the lack of white space rules not working as expected gets.

Either way, appreciate you exploring this. Side: I hope we're lucky enough to see movement on estools/escodegen#203 for newlines at some point. That would definitely be better than the situation today.

For now, at least in my own usage and in plugins I'm writing using fixmyjs, I'm recommending using legacy mode as a default (understanding limitations) and explaining what you get if you opt out of it as long as newline support isn't an issue.

@appleboy
Copy link

appleboy commented Oct 3, 2014

+1 agree with @addyosmani

Please using legacy mode as a default.

@jon-mcmillan
Copy link

My use case is a single/infrequent usage on some legacy code.

I've worked around the problem by dumping the patch file and then running a small script over the patch file to not delete the empty lines.

My script produced a few issues I had to fix by hand or I'd share it. It was still much less work than doing the js cleanup by hand though. Hopefully the idea can help some of you while the issue is resolved.

@goatslacker
Copy link
Member

I'm closing this in favor of #110 since that issue will solve this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants