Change default encoding in XML.save to UTF-8 #122

Rogach · 2016-11-10T20:33:41Z

Was ISO-8859-1, which resulted in encoding errors at runtime if document
contained non-latin1 characters. Also XML spec states that
documents without xml declaration are expected to contain UTF-8 or UTF-16,
so writing in ISO-8859-1 without xml declaration (which was the default)
can easily break compliant parsers.

ashawley · 2016-11-10T20:39:06Z

Is it worthwhile to add a scala.deprecated annotation to the method?

Rogach · 2016-11-10T20:42:12Z

@ashawley In that case we will need to create another method (saveFile, for example) and apply the change there. Is it worth it?

ashawley · 2016-11-10T20:47:56Z

Oh, right. I didn't mean deprecate. Sorry about that. Is there a way to emit a warning that the behavior of the method has changed?

Rogach · 2016-11-10T20:49:35Z

@ashawley - never saw anything like that. And since scala does not have any method for silencing warnings, I doubt there is one (else everybody would be stuck looking at that warning message on each compilation).

ashawley · 2016-11-10T20:58:16Z

Ok, good points. Maybe add a note in the method's documentation about the change then? We'll have to notify the town crier as well.

Rogach · 2016-11-10T21:03:42Z

Done. Who's the town crier?

SethTisue · 2016-11-10T21:14:53Z

we can town-cry about it in the release notes for whatever Scala 2.12.x version bumps the scala-xml module version to include this

SethTisue · 2016-11-10T21:15:26Z

src/main/scala/scala/xml/XML.scala

@@ -74,6 +74,10 @@ object XML extends XMLLoader[Elem] {
   * Saves a node to a file with given filename using given encoding
   *  optionally with xmldecl and doctype declaration.
   *
+   *  Note: default encoding was ISO-8859-1 (latin1) in pre-1.0.7 scala-xml versions.
+   *  If your code depends on charaters in non-ASCII latin1 range, specify


s/charaters/characters/

Shame on me :(

Try to closely mimic bug in XML.save and XML.loadFile, but write tests that don't use the file system. Will fail in 1.0.6 and earlier: expected:<...klmnopqrstuvwxyz{|}~[ ¡¢£¤¥¦§¨©ª«¬®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ]</x>> but was:<...klmnopqrstuvwxyz{|}~[????????????????????????????????????????????????????????????????????????????????????????????????]</x>> Will be fixed in scala#122.

ashawley · 2016-11-11T06:43:13Z

Unit test in another PR that will fail, but will be fixed by this PR.

Rogach · 2016-11-11T06:45:47Z

@ashawley Should I include that test in this PR?

ashawley · 2016-11-11T06:55:12Z

Sure. Feel free to cherry-pick or rebase in here.

Try to closely mimic bug in XML.save and XML.loadFile, but write tests that don't use the file system. Will fail in 1.0.6 and earlier: expected:<...klmnopqrstuvwxyz{|}~[ ¡¢£¤¥¦§¨©ª«¬®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ]</x>> but was:<...klmnopqrstuvwxyz{|}~[????????????????????????????????????????????????????????????????????????????????????????????????]</x>> Will be fixed in scala#122.

Rogach · 2016-11-16T09:23:20Z

Is there something else I need to do to get this PR accepted?

SethTisue · 2017-02-04T15:48:49Z

needs rebase now. LGTM otherwise and I would suggest we merge in a few days, after the rebase, if there are no actual objections

Try to closely mimic bug in XML.save and XML.loadFile, but write tests that don't use the file system. Will fail in 1.0.6 and earlier: expected:<...klmnopqrstuvwxyz{|}~[ ¡¢£¤¥¦§¨©ª«¬®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ]</x>> but was:<...klmnopqrstuvwxyz{|}~[????????????????????????????????????????????????????????????????????????????????????????????????]</x>> Will be fixed in scala#122.

Rogach · 2017-02-04T16:39:06Z

Done.

Seems rebase loses commit times, sadly (I'm more used to merge workflow).

SethTisue · 2017-02-04T16:55:45Z

Seems rebase loses commit times, sadly

are you sure? I don't think so

(regardless, thanks for taking care of it)

Rogach · 2017-02-04T17:05:05Z

I see it now - my log viewer was set up to display commiter (%cr) date instead of author (%ar). I fixed it, thanks for pointing it out!

ashawley · 2017-02-04T19:11:18Z

Still LGTM!

ashawley

LGTM

ashawley · 2017-05-24T18:20:25Z

If you could do one more rebase and push, I'll merge this, tomorrow.

Was ISO-8859-1, which resulted in encoding errors at runtime if document contained non-latin1 characters. Also XML spec states that documents without xml declaration are expected to contain UTF-8 or UTF-16 - so writing in ISO-8859-1 without xml declaration (which was the default) can easily break compliant parsers.

Try to closely mimic bug in XML.save and XML.loadFile, but write tests that don't use the file system. Will fail in 1.0.6 and earlier: expected:<...klmnopqrstuvwxyz{|}~[ ¡¢£¤¥¦§¨©ª«¬®¯°±²³´µ¶·¸¹º»¼½¾¿ÀÁÂÃÄÅÆÇÈÉÊËÌÍÎÏÐÑÒÓÔÕÖ×ØÙÚÛÜÝÞßàáâãäåæçèéêëìíîïðñòóôõö÷øùúûüýþÿ]</x>> but was:<...klmnopqrstuvwxyz{|}~[????????????????????????????????????????????????????????????????????????????????????????????????]</x>> Will be fixed in scala#122.

Rogach · 2017-05-24T18:28:39Z

@ashawley - Done.

As a side note, I have huge troubles with rebase workflow - in this case, for example, I had to rebase the changes, thus rewriting history, and then had to do push -f to github (which is usually frowned upon). How rebase workflow is supposed to work in case of several distributed repositories?

ashawley · 2017-05-24T18:37:29Z

Thanks for responding so quickly. And apologies, for troubling you by repeatedly asking you to rebase. Especially, if you aren't used to our git workflow.

Yes, rebase does rewrite git history on your branch. It looks like your branch name for this PR is called master. Is that the reason you have trouble? Usually, I create a new branch called utf-8-encoding or fix-121. That way I can

git checkout master
git pull upstream master
git checkout fix-121
git rebase master
git push -f origin fix-121

Hope that helps!

Rogach · 2017-05-24T18:42:43Z

Thanks for the explanation!

No, master branch does not give much trouble (though it will be a little bit more convenient if I did name the PR branch differently). But the thought of routinely doing push -f scares me a lot (and different branch name does not save from that). Merge workflow results in a little dirtier history, but saves me from messing with history rewriting.

ashawley · 2017-05-24T18:49:17Z

Yeah, push force is weird. In addition to git reflog, most of the git commands leave a trace in thehir output of what sha hashes you're moving between, so you can always recover or study them if you need to. I am also paranoid, so I ocassionally inspect my commits and diffs after a rebase. Although, over time and greater understanding you eventually learn to trust what rebase does, and git generally, more and more.

Rogach mentioned this pull request Nov 10, 2016

ISO-8859-1 is used as default encoding #121

Closed

SethTisue reviewed Nov 10, 2016

View reviewed changes

ashawley mentioned this pull request Nov 11, 2016

Add test for ISO-8859-1 defect found with XML.save in #121 #123

Closed

Rogach force-pushed the master branch from 268bac1 to b1fee97 Compare February 4, 2017 16:38

SethTisue approved these changes Feb 15, 2017

View reviewed changes

ashawley approved these changes May 24, 2017

View reviewed changes

Rogach and others added 4 commits May 24, 2017 21:25

Add note about default encoding change to XML.save method documentation.

9b85c9d

fix typo in method documentation

bcec742

Rogach force-pushed the master branch from b1fee97 to c416c4e Compare May 24, 2017 18:25

ashawley merged commit c23ffd6 into scala:master May 25, 2017

ashawley mentioned this pull request Oct 9, 2017

Publish release as 1.1.0 #156

Closed

3 tasks

som-snytt mentioned this pull request Apr 16, 2022

Prefer StandardCharsets scala/scala#10004

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change default encoding in XML.save to UTF-8 #122

Change default encoding in XML.save to UTF-8 #122

Rogach commented Nov 10, 2016 •

edited

Loading

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

SethTisue commented Nov 10, 2016 •

edited

Loading

SethTisue Nov 10, 2016

Rogach Nov 10, 2016

ashawley commented Nov 11, 2016

Rogach commented Nov 11, 2016

ashawley commented Nov 11, 2016

Rogach commented Nov 16, 2016

SethTisue commented Feb 4, 2017 •

edited

Loading

Rogach commented Feb 4, 2017

SethTisue commented Feb 4, 2017

Rogach commented Feb 4, 2017

ashawley commented Feb 4, 2017

ashawley left a comment

ashawley commented May 24, 2017

Rogach commented May 24, 2017

ashawley commented May 24, 2017

Rogach commented May 24, 2017

ashawley commented May 24, 2017

Change default encoding in XML.save to UTF-8 #122

Change default encoding in XML.save to UTF-8 #122

Conversation

Rogach commented Nov 10, 2016 • edited Loading

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

ashawley commented Nov 10, 2016

Rogach commented Nov 10, 2016

SethTisue commented Nov 10, 2016 • edited Loading

SethTisue Nov 10, 2016

Choose a reason for hiding this comment

Rogach Nov 10, 2016

Choose a reason for hiding this comment

ashawley commented Nov 11, 2016

Rogach commented Nov 11, 2016

ashawley commented Nov 11, 2016

Rogach commented Nov 16, 2016

SethTisue commented Feb 4, 2017 • edited Loading

Rogach commented Feb 4, 2017

SethTisue commented Feb 4, 2017

Rogach commented Feb 4, 2017

ashawley commented Feb 4, 2017

ashawley left a comment

Choose a reason for hiding this comment

ashawley commented May 24, 2017

Rogach commented May 24, 2017

ashawley commented May 24, 2017

Rogach commented May 24, 2017

ashawley commented May 24, 2017

Rogach commented Nov 10, 2016 •

edited

Loading

SethTisue commented Nov 10, 2016 •

edited

Loading

SethTisue commented Feb 4, 2017 •

edited

Loading