Skip to content

migrate to github.com/pelletier/go-toml/v2#1018

Open
thaJeztah wants to merge 2 commits into
zmap:masterfrom
thaJeztah:toml_v2
Open

migrate to github.com/pelletier/go-toml/v2#1018
thaJeztah wants to merge 2 commits into
zmap:masterfrom
thaJeztah:toml_v2

Conversation

@thaJeztah
Copy link
Copy Markdown
Contributor

@thaJeztah thaJeztah commented Feb 2, 2026

With a bit of help of ChatGPT to replace the namespace bits; ⚠️ I'm not very familiar with this module / project, so not sure exactly what kind of config to expect, but tests seemed to be happy, so 😅 🤞

@thaJeztah thaJeztah mentioned this pull request Feb 4, 2026
@thaJeztah thaJeztah marked this pull request as ready for review February 15, 2026 18:08
@thaJeztah
Copy link
Copy Markdown
Contributor Author

oh! Failure; I could've sworn it was happy locally; will have a look later 🤔

--- FAIL: TestNewGlobalWithPrivateMembersDontGetPrinted (0.00s)
    configuration_test.go:1022: wanted '[this_is_a_test]
        A = 1
        B = "2"
        ' but got '[this_is_a_test]
        A = 1
        B = '2'
        '
FAIL

@thaJeztah
Copy link
Copy Markdown
Contributor Author

Oh! just single-quotes vs double quotes; should be fixed now 👍

- B = "2"
+ B = '2'

Comment thread v3/lint/configuration.go
}

cur := any(doc)
for part := range strings.SplitSeq(namespace, ".") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I can see why it would think to do nonsense like this due to the word namespace.

So namespace is what this TOML library calls an object name.

[this_is_a_namespace]
flip_widget = true

[this_is_another_namespace]
fidget_threshold = 42

Since it's using a map as intermediary, what it really should be looking for are object keys that match the give namespace.

{
    "A": {},
    "B": {
        "C": {
            "some_config": true
         }
    }
}
got := getNamespace(doc, "C")

Should return...

{
     "some_config": true
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks! I recall now that I was not sure what the expectation was; the examples I could find were all a flat config with no nesting.

What's the expectation if there's nesting? Say;

[namespace]
C = hello

[namespace.sub1.sub2]
C = world

Would got := getNamespace(doc, "C") traverse the tree and find the first match (because that looks ambiguous); I gave it a quick try, and it looks like currently it would allow namespace to be namespace.sub1.sub2 to resolve a config.

Let me push a test for what I had in mind.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Comment on lines +27 to +51
func TestNestedConfigs(t *testing.T) {
c, err := NewConfigFromString(`
[Test]
A = "Test"
[Test.Sub1]
A = "Test.Sub1"
[Test.Sub1.Sub2]
A = "Test.Sub1.Sub2"
[Test2.Sub3.Sub4]
A = "Test2.Sub3.Sub4"
[Test3.ConfigA.ConfigB]
A = "Test3.ConfigA.ConfigB.A"
`)
if err != nil {
t.Fatal(err)
}
type Test struct {
A string
}
type Test2 struct {
ConfigA struct{ ConfigB Test }
}
t.Run("nested configs", func(t *testing.T) {
for _, ns := range []string{"Test", "Test.Sub1", "Test.Sub1.Sub2", "Test2.Sub3.Sub4"} {
test := Test{}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a commit with a tests that passes before and after this PR 😅 - perhaps you could have a peek to see if this test matches the intended behavior 🤗

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants