-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Unmarshal keys containing dots #673
Unmarshal keys containing dots #673
Conversation
Hi can you please rebase this into once commit? So we stand a chance to get this reviewed fast and merged? |
Sure thing, will do it when I get home this afternoon. |
We have very similar code in our fork: https://github.com/DataDog/viper/pull/2/files |
@albertvaka yes looks very similar, although yours uses the |
Hello, any update on this? I also encountered this issue |
Waiting for spf13#673 to be merged
Hello @inkychris, any update on this PR? How can I help to make it merge? |
@silvin-lubecki As far as I'm aware, this should be okay to merge; it's just waiting on a maintainer to review/approve. |
This looks good. Can you rebase against tip and I'll gladly merge. |
@spf13, this should be good to go now, thanks! |
ping @spf13 😺 |
please merge @spf13 |
@spf13 @sagikazarmark any chance one of you could merge this please? I'd rather not have to keep rebasing... Thanks! |
I'll try to take a look at it soon, but no promises. :) |
@sagikazarmark ping ;-) |
Hopefully someone can review this and merge, its so annoying to be stuck on a old forked patched version :( I have settings for "domains", domains always contains dot's... |
I started reviewing this PR. I like the idea and the implementation, but I suspect there might be an edge case where this PR introduces a backwards incompatible change:
In this case I would expect that @inkychris can you add a test case (probably with a different email address) that verifies the original behavior remains intact in such cases? Otherwise 👍 and sorry you had to wait so much for moving forward with this. Thanks @inkychris for the PR. |
For me, the original behaviour results in the value of that key being package viper
import (
"github.com/stretchr/testify/assert"
"strings"
"testing"
)
const yamlConfig = `
emails:
steve@hacker:
com:
created: 01/02/04
active: false
steve@hacker.com:
created: 01/02/03
active: true
`
func TestConflict(t *testing.T) {
SetConfigType("yaml")
assert.NoError(t, ReadConfig(strings.NewReader(yamlConfig)))
assert.True(t, GetBool("emails.steve@hacker.com.active"))
} I should also note I'm testing on Windows with go 1.12 in case this is something that arises from un-ordered parsing or something? I would expect the issue here is that the address Apologies, it's been a little while since I thought about this so trying to get up to speed again; thanks for looking at it! |
Hm, I guess you are right. After thinking it through again, it makes sense. Otherwise the fix itself wouldn't work in the first place, since it builds on keys being |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @inkychris
thanks @sagikazarmark and @inkychris |
Changed formatting of test objects for better git diffing and readibility. Fixed failing tests on Windows.
Any update on this? Can this be merged? |
Yup, I think it's good to go. |
Well, it seems this PR did introduce a regression after all. Dot-notated keys so far were always parsed as structures. Sources, like manually set values, defaults and flags always use dot notated keys, compared to config files:
In this case there is no way to know the original intention (is the key At this point we either revert this PR, or change it's scope to unmarshal only and generate a map that's compatible with both approaches, although that would potentially be much slower than the current solution. |
In order to support both use-cases (I have an usecase where some keys contain |
setting keyDelimiter is fine with me, no solution would be bad |
I submitted a PR reverting the effective changes of this PR (I left the reformatting in place). Sorry folks, this cannot land in the current version. 🙁 I will send another PR for setting the key delimiter, although we will have to check first if it has any unwanted effects. |
The underlying issue was conceptual (not being able to determine the key path components from it's fully qualified path) which is only really solvable by either escaping or changing the delimiter used, so it's understandable. Just a heads up for the following PR that the following line appears to have missed the Line 1441 in 99520c8
|
Thanks for the heads up @inkychris ! |
Fixes #324
Includes fix #669
Replaced the use of
strings.Split
with a method that takes a fully qualified key and returns a list of valid map keys which may include dots in them.Reformatted test objects to improve readability and Git change diffing.
Added key containing dot to yaml test.