New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Odd handling of quotes #2

Closed
cactus opened this Issue Oct 1, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@cactus

cactus commented Oct 1, 2015

If I modify decode_test.go with the following...

const configString = `
# This is a comment

# An integer value
answer = 42

# A float value
pi = 3.14
# A boolean value
is_active = true

# A string value
quotes = Alea iacta est\nEt tu, Brute?

# a string in quotes
quoted_string = "this is a string too!"
`

Then adjust the tests to handle that case, I see this result.

--- FAIL: Test_Unmarshal (0.00s)
    decode_test.go:70: Expected "this is a string too!", got "\"this is a string too!\""
--- FAIL: Test_UnmarshalFromConfig (0.00s)
    decode_test.go:135: Expected "this is a string too!", got "\"this is a string too!\""

Is this expected? Most other "INI-like" formats strip quotes from the front and back of strings in this case.

Adding this to GetString makes it behave as I would expect.

diff --git a/config.go b/config.go
index f7ebdfe..38dfd3f 100644
--- a/config.go
+++ b/config.go
@@ -50,6 +50,9 @@ func (c *Config) GetString(key string) (string, error) {
    }

    uq := strings.Replace(val, "\\n", "\n", -1)
+   if strings.HasPrefix(uq, `"`) && strings.HasSuffix(uq, `"`) {
+       uq = strings.Trim(uq, `"`)
+   }
    return uq, nil
 }
@cactus

This comment has been minimized.

Show comment
Hide comment
@cactus

cactus Oct 1, 2015

If you are amenable to the change, I can open a pull req with associated tests.

cactus commented Oct 1, 2015

If you are amenable to the change, I can open a pull req with associated tests.

@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Oct 1, 2015

Owner

Thanks for reporting!

I would say that this is expected behaviour, the only character that is special is the line break. Because it would break the line based format if it isn't escaped. But if you write quotes you get quotes :)

This keeps down the number of special cases. E.g. what to do about single quotes, unicode quotes (the quotes that looks like quotes but isn't) and so on. This way everything you put in the key/value will be there. The one special case is line breaks.

But you are right that it's a bit confusing. I will add a test case that verifies that this is the intended behaviour.

If I'm missing something here please tell me.

Owner

walle commented Oct 1, 2015

Thanks for reporting!

I would say that this is expected behaviour, the only character that is special is the line break. Because it would break the line based format if it isn't escaped. But if you write quotes you get quotes :)

This keeps down the number of special cases. E.g. what to do about single quotes, unicode quotes (the quotes that looks like quotes but isn't) and so on. This way everything you put in the key/value will be there. The one special case is line breaks.

But you are right that it's a bit confusing. I will add a test case that verifies that this is the intended behaviour.

If I'm missing something here please tell me.

@walle walle closed this Oct 1, 2015

walle added a commit that referenced this issue Oct 1, 2015

Add test case for quoted string behaviour
Add a test case that shows how the code is supposed to handle quoted
strings in configuration values.
Addresses issue #2.
@cactus

This comment has been minimized.

Show comment
Hide comment
@cactus

cactus Oct 1, 2015

Maybe adding some clarifying text to the readme would help too?

cactus commented Oct 1, 2015

Maybe adding some clarifying text to the readme would help too?

@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Oct 1, 2015

Owner

Great idea! I will do so.

Owner

walle commented Oct 1, 2015

Great idea! I will do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment