scala-ide / scala-worksheet

A Scala IDE plugin for a multi-line REPL (called worksheet)
96 stars 24 forks source link

Use project encoding when saving worksheet #124

Closed megri closed 11 years ago

megri commented 11 years ago

Closes #120

dragos commented 11 years ago

@megri, thank you for your contribution! I wonder if you could add a small test-case, as I imagine you have one already. There are tests in the .tests bundle, it shouldn't be very difficult to add one. Let me know if you need help!

megri commented 11 years ago

would that do it?

dragos commented 11 years ago

Awesome! Looks really good to me. Before we can merge your pull request, there's one more thing: we need you to sign the Contributor License Agreement. It's an entirely automatic process, done using your github id, and can be done here.

megri commented 11 years ago

Done.

dotta commented 11 years ago

:+1:

Though, this commit looks funny. I think the easiest thing to do is to rebase on master and squash your two commits together. Also, when you squash the commits, would you mind adding something like Fixes #120 in your commit message.

megri commented 11 years ago

I'm not totally sure how I just did that but I think I got it..

dotta commented 11 years ago

@megri It's almost perfect, your commit message only misses a title.

For an example of what I mean, please look here (again, it's my bad for not having a CONTRIBUTING doc in the repository, I'll make sure to add one in the next days. Sorry.).

megri commented 11 years ago

No problems, you live and learn ;)

Writing up the detailed description made me think there ought to be a rock solid way of fixing this, but I guess this is ok for the small effort involved.

dotta commented 11 years ago

Love the attitude! This is good to :sheep:

So, what's your next pick? You didn't really think we were gonna let you leave after such a great contribution, did you? ;)

megri commented 11 years ago

Actually, I might have broken test compilation for scala < 2.10 since I'm using string interpolation in the test code.. Maybe I should fix that..

dotta commented 11 years ago

@megri Right. I can fix it, no worries (I'm doing some Spring clean up, so I can take care of this as well ;)).

dotta commented 11 years ago

@megri Unfortunately, I had to re-open #124. The reason is that the test doesn't pass from the terminal on MacOSX (while it works perfectly fine from inside Eclipse, as I force eclipse to start with -Dfile.encoding=UTF8). It would be great if you have a look at my PR https://github.com/scala-ide/scala-worksheet/pull/127. Maybe you have a hunch on how we could fix the test?

megri commented 11 years ago

Sorry, I just now noticed this. I'll have a look in the next few days :)

dotta commented 11 years ago

@megri No problem!

megri commented 11 years ago

@dotta, @dragos: What do you guys think of this for a test:

  1. Keep "troublesome" unicode text in a resource file;
  2. Read file into String using correct encoding (scala.io.Source should work);
  3. Use worksheet engine to compile back to disk;
  4. [Byte-]compare result against a fabricated "expected results"-file (should line endings be considered?)
dotta commented 11 years ago

@megri The idea looks good to me. This way we could make sure that the system encoding is created for both creating and executing the instrumented source. However, I'm not sure that the current design will allow you to easily create the test :-/ Some (involved?) refactoring may be needed.

Also, I just realized there is a workaround that may be acceptable. In fact, it's enough to pass the VM argument -Dfile.encoding=UTF-8 to the (Tycho) task that executes the tests. This is not to say that we wouldn't accept a contribution for fixing the issue properly, but maybe there are more fun issues in the worksheet that could get your attention :) (it's really up to you)