Fixing Program/BlockStatement rewrites #100

Closed
wants to merge 3 commits into
from

2 participants

@jimfleming

Fixes #90

There is one failing test I haven't figured out how to fix yet. We need to unwrap ExpressionStatements for the general case but here if we don't then we end up with two semicolons.

@coveralls

Coverage Status

Coverage decreased (-4.75%) when pulling e000c0d on jimfleming:block_statement into 358532a on rdio:master.

@coveralls

Coverage Status

Coverage decreased (-1.8%) when pulling 376b24a on jimfleming:block_statement into 358532a on rdio:master.

@jimfleming jimfleming referenced this pull request Aug 8, 2014
Closed

v0.4.0 #110

@brettlangdon brettlangdon commented on the diff Aug 9, 2014
tests/rewrite.js
@@ -40,7 +67,7 @@ describe('jsfmt.rewrite', function() {
// Inside of "BlockStatement" instead of "Program"
jsfmt.rewrite('function test() { var myA = 1, myB = 2; }', 'var a = c, b = d; -> var a = c; var b = d;')
- .toString().should.eql('function test() {\n var myA = 1;\n var myB = 2;\n}');
+ .toString().should.eql('function test() { var myA = 1;\nvar myB = 2; }');
@brettlangdon
brettlangdon added a line comment Aug 9, 2014

this doesn't seem exactly right:

function test() { var myA = 1, myB = 2; }

gets rewritten to:

function test() { var myA = 1;
var myB = 2; }

is this the actual expected? I would assume it shouldn't get the newline?

@jimfleming
jimfleming added a line comment Aug 12, 2014

Hmm, yah. So, what happens now is that when rewriting it only mangles the matched expressions, without context. So only the matched expressions will be formatted, post-rewrite. It may make sense to explicitly re-format the entire output. What do you think?

@brettlangdon
brettlangdon added a line comment Aug 12, 2014

well, we do that now dont we with rewriting? it'll reformat unless you give --no-format?

@jimfleming
jimfleming added a line comment Aug 12, 2014

Hmm, good point. So this is just the raw output from the rewrite before the formatting has been applied.

@brettlangdon
brettlangdon added a line comment Aug 12, 2014

which actually might be ok, since we dont intend on testing formatting here, we want to test rewrite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brettlangdon brettlangdon commented on the diff Aug 9, 2014
tests/rewrite.js
@@ -60,6 +87,6 @@ describe('jsfmt.rewrite', function() {
});
it('should rewrite AssignmentExpression', function() {
- jsfmt.rewrite('var test = 4;', 'var a = b -> a += b').toString().should.eql('test += 4;');
+ jsfmt.rewrite('var test = 4;', 'var a = b -> a += b').toString().should.eql('test += 4');
@brettlangdon
brettlangdon added a line comment Aug 9, 2014

why do we lose the semicolon? shouldn't that stay?

@jimfleming
jimfleming added a line comment Aug 12, 2014

Same reason as above. Its only matching the inner-expression (the part we care about) not the statement (the part that owns the semicolon). This is related to the test-failure where it creates a semi-colon for the matched expression (due to its own local context) but there was already a semi-colon there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jimfleming

Unfortunately I haven't had the time to delve into this (I think its at least a day of work to sort it out). Some thoughts:

  • Maybe we don't need to unwrap ExpressionStatement? I feel like I tried this and it caused different problems but we have more tests now.
  • Maybe this is a more fundamental problem with how I'm doing the rewriting.
@jimfleming

Needs a rewrite.

@jimfleming jimfleming closed this May 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment