millermedeiros / esformatter

ECMAScript code beautifier/formatter
MIT License
970 stars 91 forks source link

we should probably not remove original indentation #38

Closed millermedeiros closed 10 years ago

millermedeiros commented 11 years ago

right now I remove trailing whitespace and indentation before processing the tokens (using RegExp) I think a better approach would be to convert the simple WhiteSpace tokens that are at the start of the line to an Indent token (without changing the amount of chars or anything like that - just set the token.type) and make our indentation code simply replace the amount of chars (or add a new token if it doesn't exist).

That will probably make it easier to implement #10 and #12 and will also make it easier to keep user indentation (if needed).

Asuza commented 11 years ago

I would like to see this feature. Right now if I set all of the "indent" options to 0, I would expect it to not do anything with indentation. Instead, it removes all of the existing indentation which, at least for me, is undesirable.

millermedeiros commented 11 years ago

@Asuza I think we need another option besides 0 and 1 like described on #1

The proper way to implement this is on the util/indent. We don't need to remove the original indentation, we just need to replace the WhiteSpace token type and value (if it already exists) or add a new Indent token on indent.before() - since indentation is centralized on the before method that is the only place we need to change and we can delete the line that does the RegExp.

ruyadorno commented 11 years ago

I'll be working on it next

jzaefferer commented 11 years ago

WIP from @ruyadorno https://github.com/ruyadorno/esformatter/compare/iss-38 (since that wasn't referenced here)

jzaefferer commented 11 years ago

I've tested that branch locally and there's actually just three comments in the tests that get bad indentation. One of them (in the while_statement test) appears when the previous node has whitespace in front of the closing brance:

while(true) {
log(n)
 }
// no body
  x;

Remove that space in front of the } and the comment doesn't get indented.

This also happens in the try_statement test:

try{}
catch (e){
log(e)
 }

// issue #35: "catch" block indent + empty catch body
x

Again, removing that space in front of the } and the comment doesn't get indented.

I have no clue why this happens. The rocambole visualizer shows the same nodes and tokens for both (with and without the space).

I also ran this against the tests from #58. The comments with really bad indentation still have bad indentation, but I suppose that's to be expected.

@millermedeiros could you take it from here?

ruyadorno commented 11 years ago

If @millermedeiros still too busy I'll resume work in september once I get back to Montreal :)

jzaefferer commented 11 years ago

@ruyadorno are going to work on this again?

ruyadorno commented 11 years ago

I still quite busy setting things up here, if anyone is willing to continue, please, just go ahead. But yes, I do plan to resume work on esformatter as soon as I can.

millermedeiros commented 11 years ago

I tried to implement this yesterday for a couple hours but failed, it is not that simple to do a large refactor like this since we have a lot of edge cases (many tests broke and some of then are hard to fix). I'm guessing I'll need at least two full days to make this work.

I'll make a huge effort to implement it this month and will push to a new branch if I make any promising progress.