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

Block statements #47

Merged
merged 9 commits into from Jun 3, 2014
Merged

Block statements #47

merged 9 commits into from Jun 3, 2014

Conversation

jimfleming
Copy link
Contributor

If someone could play around with this (test it out) I'd appreciate it. I've added some tests and it seems to work great but I want to make sure I didn't miss any edge cases.

This fixes #28

@brettlangdon
Copy link
Contributor

Latest version of jsfmt

$ echo 'var a = 1, b = 2;' | jsfmt -r 'var a = b, c = d -> var a = d, b = c'
var a = 2,
  b = b;

Your version:

$ echo 'var a = 1, b = 2;' | ./bin/jsfmt -r 'var a = b, c = d -> var a = d, b = c'
stdin { [Error: Line 1: Unexpected number] index: 11, lineNumber: 1, column: 12 }

expected?

@jimfleming
Copy link
Contributor Author

Hmm, good question. The second one sounds more correct. I think I'm missing an "Identifier" check.

@jimfleming
Copy link
Contributor Author

I can duplicate your results from the command-line. I haven't looked at it in detail yet but, interestingly, I get a different result from the api:

jsfmt.rewrite('var a = 1, b = 2;', 'var a = b, c = d -> var a = d, b = c')
  .toString().should.eql('var a = 2, 1 = b;');

EDIT: Part of the problem is that the result, in either case, is not syntactically correct.

@jimfleming
Copy link
Contributor Author

Ok, so the first command you posted is actually producing incorrect output. I believe the correct output is 'var a = 2, 1 = b;'.

The new code produces the correct output (however is syntactically invalid). When using the api it will output the correct result. When using the command-line you get a syntax error if formatting (pass --no-format to see the same output as the api).

EDIT: If we wanted to validate the rewritten javascript we could attempt to parse the rewritten code with esprima and throw the error if its invalid. This will slow it down a little and re-parse when formatting from the command-line but maybe worth the check?

@brettlangdon
Copy link
Contributor

@jimfleming re: validating, if we enable formatting on rewrite then it will get parsed by esprima right? or do we do the rewrite while formatting? maybe we can add a cli argument to force re-checking?

@jimfleming
Copy link
Contributor Author

A new argument is a good idea. Should I update this PR or post another?

On Thursday, May 29, 2014, Brett Langdon notifications@github.com wrote:

@jimfleming https://github.com/jimfleming re: validating, if we enable
formatting on rewrite then it will get parsed by esprima right? or do we do
the rewrite while formatting? maybe we can add a cli argument to force
re-checking?


Reply to this email directly or view it on GitHub
#47 (comment).

Jim

@brettlangdon
Copy link
Contributor

@jimfleming a new PR probably. how do we expect it will work. Assume reparse (+ format if they want) before actually writing the rewrite to the file?

@jimfleming
Copy link
Contributor Author

@brettlangdon Yah, if its by option I don't mind the reparsing — I can't think of a clean way with the current arch to pass the pre-parsed version to formatting.

Avoiding the reparsing would require separating out the ast generation from formatting and a format function that accepts just the AST with the exposed api function being a combination of the two (I think). Actually, that might be ok.

EDIT: http://esprima.org/demo/validate.html

@brettlangdon
Copy link
Contributor

hmmm, interesting, think I can get on board for this feature.

Maybe add it as an option that can stand on it's own?

jsfmt --no-format --validate my-script.js
# show me any errors

@jimfleming
Copy link
Contributor Author

#49

@jimfleming jimfleming mentioned this pull request Jun 3, 2014
@brettlangdon
Copy link
Contributor

@jimfleming where does the pull request stand? now that we added --validate is this ready to be merged or are there other tests we want to run with it?

@jimfleming
Copy link
Contributor Author

@brettlangdon From what I can tell this is good to go, assuming you agree. It functions as expected now.

@brettlangdon
Copy link
Contributor

+1

jimfleming added a commit that referenced this pull request Jun 3, 2014
@jimfleming jimfleming merged commit a81dc43 into rdio:master Jun 3, 2014
jimfleming added a commit to jimfleming/jsfmt that referenced this pull request Jun 10, 2014
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

Successfully merging this pull request may close these issues.

Cannot expand comma-last variable notation
2 participants