ruby-rewrite fails to actually rewrite the input file when a rewriter is loaded. This fixes the issue.

[MrJoy]

At the end of the @rewriters.each loop, the value of buffer.name is no longer valid due to debugging annotations:

        new_buffer = Source::Buffer.new(initial_buffer.name +
                                    '|after ' + rewriter_class.name)

This preserves the original name, allowing the file to be properly rewritten.

[MrJoy]

Test case to follow as soon as I figure out the right strategy for it. Suggestions on your preferred approach would be greatly appreciated.

[whitequark]

Your commit message is a bit confusing... could you use something like "- Always rewrite source file with original filename regardless of returned AST" ?

Also, you don't have to recreate the PR. You can force-push the branch (git push -f origin fix_file_rewriting_with_rewriters) and it will update the PR.

I think you can add a rewriter that would trigger the bug, and invoke the runner merely with "system", and then verify that the runner succeeded and the correct file was changed. You could name the file test/test_runner_rewrite.rb, and the sample runner test/rewriter_bug_162.rb or something like that.

Thanks!

[MrJoy]

The bug is that any rewriter at all will cause the file to not be rewritten. So the message "Always rewrite source file with original filename regardless of returned AST" is a touch misleading as it doesn't actually have anything to do with the AST.

The approach you suggest is what came to mind after perusing the test suite. Glad to hear you find it acceptable! I'll also need a fixture file of course, so I'm inclined to bundle all three files into a sub-directory under test/. Say, test/bug_XXX (I'll file a proper issue momentarily and reference it from here) with files like bug_XXX_rewriter.rb, test_runner_rewrite.rb, and bug_XXX_input.rb. I'll FileUtils.mkdir('test/tmp') and copy the source file in, then assert that the original andtmp/` versions should not be identical.

[MrJoy]

(And once we suss out ideal commit message, I'll do a force-push here instead of re-submitting again...)

[MrJoy]

Bug filed as #163

[MrJoy]

I've confirmed the new test case fails passes with the fix by running:

ruby test/bug_163/test_runner_rewrite.rb

And that it fails without the fix by running:

git revert --no-commit 0e4e2037f11300f2c90f53c817e19f085999012e
ruby test/bug_163/test_runner_rewrite.rb

[MrJoy]

In light of my clarification, suggested commit message?

[MrJoy]

Clarifications provided.

[MrJoy]

Erk. Travis shows green now: https://travis-ci.org/whitequark/parser/builds/33318214

[whitequark]

Please use Dir.mktmpdir for this.

[whitequark]

Also, the commit messages for the tests shouldn't go into the changelog--so you need to remove the "- " prefix for them.

Otherwise, your PR is fine.

[MrJoy]

Will fix these up. Can you run travis cache --delete to see if that fixes the Rubinius issue?

[whitequark]

@MrJoy That wouldn't work afaik, we'll need to wait until Travis updates images.

[MrJoy]

Ok. I'm also tossing in a slight tweak that is apparently (waiting on Travis to confirm) harmless elsewhere but seemed to be needed for me to run the tests under Rubinius locally. I can back it out if you don't want. (See commit 1c3237d)

[MrJoy]

Except for Rubinius issue, all's green in Travis.