Change overwrite and remove to remove interior inserts #89
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changes the `overwrite` and `remove` behavior to be consistent with `move` and `slice`: the specified range includes any `insertRight`s on the left side and any `insertLeft`s on the right side. The code change ended up actually making the code simpler: `Chunk.edit` now overwrites all chunk content, including the intro and outro, so the `overwrite` code doesn't need to be careful about clearing the intro and outro for specific chunks. This fixes decaffeinate/decaffeinate#269 . See my comment in that bug for an explanation of what was going wrong, and why this case is important. In addition to this passing all of the magic-string tests, I also ran all of the decaffeinate tests with this change and they all pass. So at least with the decaffeinate project, the code wasn't depending on the previous behavior.
alangpierce
force-pushed
the
fix-overwrite-remove-ranges
branch
from
b8c01fa
to
3f6dd4d
Compare
alangpierce
added a commit
to alangpierce/decaffeinate
that referenced
this pull request
Jul 10, 2016
Before this commit, the comment after a removed return statement would be destroyed: http://decaffeinate-project.org/repl/#?evaluate=true&code=-%3E%0A%20%20a%20%20%23%20foo%0A%20%20return Now, the code is a little more careful by deleting up to the start of the line or the previous semicolon. Also, the code now exits early and doesn't patch the `return` node or insert a semicolon at the end, since that was causing a semicolon at the end of the comment. This also fixes a crash on this type of input that I ran into when using magic-string with Rich-Harris/magic-string#89 applied.
eventualbuddha
pushed a commit
to decaffeinate/decaffeinate
that referenced
this pull request
Jul 10, 2016
Before this commit, the comment after a removed return statement would be destroyed: http://decaffeinate-project.org/repl/#?evaluate=true&code=-%3E%0A%20%20a%20%20%23%20foo%0A%20%20return Now, the code is a little more careful by deleting up to the start of the line or the previous semicolon. Also, the code now exits early and doesn't patch the `return` node or insert a semicolon at the end, since that was causing a semicolon at the end of the comment. This also fixes a crash on this type of input that I ran into when using magic-string with Rich-Harris/magic-string#89 applied.
alangpierce
added a commit
to alangpierce/decaffeinate
that referenced
this pull request
Jul 11, 2016
The code in decaffeinate#301 is actually parsed incorrectly by the CoffeeScript parser (even before decaffeinate-parser does anything interesting). It says that the function body ends at the end of the file, so decaffeinate was inserting a close-curly-brace at the end of the file, which was incorrect. However, the function application was correctly parsed, and explicitly putting parens for the function application causes the CoffeeScript parser to work again, so we can work around this issue by inserting parens for all implicit functions before the MainStage. I think this will also fix a bunch of other problems caused by implicit function calls. For example, it also happens to fix decaffeinate#269. In the codebase that I'm trying to decaffeinate, it fixes 49 out of the 104 files with decaffeinate failures that I hadn't categorized. I added the heuristic that we put the close-paren on the next line if the last arg is a multi-line expression. This fixed the formatting in almost every test, except the change introduced two test issues: * Object literal formatting sometimes had an excessive newline in non-implicit function calls ( http://decaffeinate-project.org/repl/#?evaluate=true&code=a(b%2C%0A%20%20c%3A%20d%0A%20%20e%3A%20f%0A%29 ) so adding parens exposed the issue. It's pretty minor, though. I think. * In another test, the added parens interfered with some other operations in the normalize step in a way that will be fixed by Rich-Harris/magic-string#89 Closes decaffeinate#301. Closes decaffeinate#269.
eventualbuddha
pushed a commit
to decaffeinate/decaffeinate
that referenced
this pull request
Jul 11, 2016
The code in #301 is actually parsed incorrectly by the CoffeeScript parser (even before decaffeinate-parser does anything interesting). It says that the function body ends at the end of the file, so decaffeinate was inserting a close-curly-brace at the end of the file, which was incorrect. However, the function application was correctly parsed, and explicitly putting parens for the function application causes the CoffeeScript parser to work again, so we can work around this issue by inserting parens for all implicit functions before the MainStage. I think this will also fix a bunch of other problems caused by implicit function calls. For example, it also happens to fix #269. In the codebase that I'm trying to decaffeinate, it fixes 49 out of the 104 files with decaffeinate failures that I hadn't categorized. I added the heuristic that we put the close-paren on the next line if the last arg is a multi-line expression. This fixed the formatting in almost every test, except the change introduced two test issues: * Object literal formatting sometimes had an excessive newline in non-implicit function calls ( http://decaffeinate-project.org/repl/#?evaluate=true&code=a(b%2C%0A%20%20c%3A%20d%0A%20%20e%3A%20f%0A%29 ) so adding parens exposed the issue. It's pretty minor, though. I think. * In another test, the added parens interfered with some other operations in the normalize step in a way that will be fixed by Rich-Harris/magic-string#89 Closes #301. Closes #269.
|
This seems logical – released as 0.16.0. Thanks and sorry for the long wait! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changes the
overwriteandremovebehavior to be consistent withmoveand
slice: the specified range includes anyinsertRights on the left sideand any
insertLefts on the right side.The code change ended up actually making the code simpler:
Chunk.editnowoverwrites all chunk content, including the intro and outro, so the
overwritecode doesn't need to be careful about clearing the intro and outro for specific
chunks.
This fixes decaffeinate/decaffeinate#269 . See my
comment in that bug for an explanation of what was going wrong, and why this
case is important.
In addition to this passing all of the magic-string tests, I also ran all of the
decaffeinate tests with this change and they all pass. So at least with the
decaffeinate project, the code wasn't depending on the previous behavior.