properties icon indicating copy to clipboard operation
properties copied to clipboard

Preserving the backslash character

Open jpnjfk opened this issue 4 years ago • 10 comments

This change leaves the escape character backslash '' when the backslash isn't precede one of " :=fnrt" that need escaping. Currently, any backslash that isn't precede one of the escaped characters is simply dropped. We don't know how each property value will be processed by the callers. So, it would be better that the package 'properties' doesn't make modifications as possible because the backslash may be put on purpose.

jpnjfk avatar Jul 16 '19 07:07 jpnjfk

Hmm, I get it but this would be a breaking change since this behavior has been in the library since 2014. If you need this then this should be configurable.

magiconair avatar Jul 16 '19 08:07 magiconair

Thank you for your comment. Does configurable means using an environment variable? Or callers should be able to control turning on/off the function? Of course, the default value is off. :) Is a global variable okay? I don't want to mess the existing code.

jpnjfk avatar Jul 16 '19 16:07 jpnjfk

I would expect a flag in the Properties struct which enables this, e.g. KeepBackslash, RetainBackslash or something similar which defaults to false (i.e. off).

Global variable is not ok, unfortunately. This needs to be properly integrated and there need to be tests for this.

magiconair avatar Jul 17 '19 00:07 magiconair

Thank you for your suggestion. I made a new commit whose comment is "To avoid a breaking change, the keepBackslash is false by default". That works as expected though I messed up the source codes including the tab alignments the Visual Source Code automatically did. Could you review my changes again?

jpnjfk avatar Jul 17 '19 02:07 jpnjfk

In order to propagate the flag Properties.KeepBackslash to the lexer, I had to make changes in the other structure and the argument of lex().

jpnjfk avatar Jul 19 '19 18:07 jpnjfk

I gets back the two test cases I commented out in load_test.go.

jpnjfk avatar Jul 21 '19 05:07 jpnjfk

I think that my changes are ready to go. Could you review them?

jpnjfk avatar Jul 30 '19 14:07 jpnjfk

I've thought about this a bit more. The Java properties implementation silently drops single backslash characters to avoid tripping over invalid escape sequences. The Go version emulates that behavior.

The method does not treat a backslash character, , before a non-valid escape character as an error; the backslash is silently dropped. For example, in a Java string the sequence "\z" would cause a compile time error. In contrast, this method silently drops the backslash. Therefore, this method treats the two character sequence "\b" as equivalent to the single character 'b'.

(https://docs.oracle.com/javase/7/docs/api/java/util/Properties.html#load(java.io.Reader))

However, if you escape the backslash it works as expected. What am I missing?

func TestBackslash(t *testing.T) {
	input := "k\\\\ey=va\\\\lue"
	p := mustParse(t, input)
	assert.Equal(t, p.MustGet("k\\ey"), "va\\lue")
}

magiconair avatar Aug 06 '19 02:08 magiconair

Thank you for explaining the behavior. Yes, the emulation point of view, the behavior is correct by default. As I wrote before, we don't know how each property value will be processed by the callers. So, it would be nice that the package 'properties' doesn't make silent modifications as possible because the backslash may be put on purpose. Since this is an optional behavior, the change would give the developers simplification of escaping characters in text. How do you think?

jpnjfk avatar Aug 07 '19 17:08 jpnjfk

Hi, Is it possible to add the option? What I want is an option that doesn't make silent modifications as possible. I agree that the workaround you wrote works. However, it would be error prone for translators who aren't programmers. It would be nice to have the package as simple as possible.

jpnjfk avatar Sep 03 '19 15:09 jpnjfk