[ruby-core:18995] Re: Encodings::default_internal patch

On Sun, 28 Sep 2008 02:02:57 +1000, James Gray <[e-mail]>  
wrote:

>> 2) Extended the "magic comment" feature to also look for  
>> "internal_encoding: XXXX". If this is found in the MAIN ruby file only,  
>> both the source encoding AND the default_internal are set to the given  
>> encoding. If found in an included source file (eg: library), it is  
>> treated the same way as "encoding:" and sets the source encoding only.  
>> (You shouldn't use it in a library anyhow).
>
> This is perhaps the only point I'm not loving.  The reason is that I  
> just think it would be nice if source encoding could be sorted out with  
> a single magic comment.  Could we instead just expand the current  
> comment to support the -E like syntax:
>
>    # coding: UTF-16BE:UTF-8
>
> ?  Just a thought.

Sorry, perhaps what I said was confusing.
What I meant was that you can use "internal_encoding:" instead of  
"encoding:" or "coding:" and what it will do is set BOTH the source  
encoding and default_internal encoding.
I am not sure I see the value of setting default_internal to something  
different to the source encoding, do you?
Also I am not sure what some text editors that look for "coding:" would  
make of "UTF-16BE:UTF-8".

> On a separate point, I admit that I haven't tested it yet, but I'm  
> curious what affect this will have on the work I did for CSV.  Do you  
> have any thoughts on this?
>
> I'm praying it will all just work, because CSV already checks incoming  
> encodings and honors an internal_encoding() when it is non-nil.  I'm  
> figuring this patch will just cause that value to be set when people are  
> using this feature and CSV will continue to work in their chosen  
> encoding.  Do I have that right or is this wishful thinking on my part?

Yes, libraries are still an issue. I think a couple more things should  
probably be done to make "default_internal" work better. Unfortunately I  
felt I couldn't do more in my patch at this stage, because it would not be  
backward compatible with 1.9.0. They are:

- My honest opinion is that "default_internal" should always be set to  
something by default. If a program really wants to read different file  
encodings and retain the encodings in the strings in their programs, they  
should explicitly open the file with mode "enc:-".
- What to do if "default_internal" is not ASCII compatible? The main  
problem here (as you found, James) is that string literals need to be  
encoded, which is particularly ugly. I see 2 possible solutions:
	- Insist that "default_internal" be ascii compatible, or
	- Change the Ruby parser to automatically transcode string & regexp  
literals to the "default_internal" encoding.
This would get rid of the need for code (from CSV) like:
	meta["class".encode(csv.encoding)].split("::".encode(csv.encoding))
We really have to do something about that!

So in the absence of the above, I suggest in the meantime that the  
libraries that require it have some code at the top checking  
"default_internal", eg:

	if Encoding.respond_to?(:default_internal) && (!Encoding.default_internal  
|| !Encoding.compatible?(Encoding.default_internal,  
Encoding.find("US-ASCII")))
		raise Encoding::CompatibilityError, "default_internal must be set to an  
ASCII-compatible encoding to use library XXX"
	end

for example something like this would be needed if the library uses ASCII  
string literals which need to interact with other strings.

As for CSV in particular, I think only 1 minor change will be required  
(just by a very quick inspection of the code):
- Your code to set @encoding will need to set it to "default_internal" if  
it is set, rather than "default_external"

Had James not already gone to the extraordinary lengths he did to get  
non-ascii compatible encodings working, I would have just suggested:
- put in the above test, and don't bother encoding ASCII string literals  
to match (as you will know that the encoding is ascii compatible)
- don't bother doing any string transcoding at all - let default_internal  
handle it automatically
- don't support a user-settable internal encoding

That would mean losing support for non-ASCII internal encodings, but the  
code would be much cleaner, simpler & more efficient I think.

Cheers
Mike