Skip to content
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

Fix nested values can span sections #306

Merged
merged 2 commits into from
Dec 2, 2021
Merged

Fix nested values can span sections #306

merged 2 commits into from
Dec 2, 2021

Conversation

ravron
Copy link
Contributor

@ravron ravron commented Oct 29, 2021

What problem should be fixed?

When configured with AllowNestedValues: true, the parser attempts to parse values "nested" under a key, AWS-style. To trigger this behavior, the parent key must have an empty value, and child keys must be indented. However, if the next value is in a different section, it cannot be a nested value, but the parser can treat it as such anyways. As an example:

[section]
key1 = value1
key2 =
[section2]
  key3 = value3

Before this PR, key3 is treated as a nested value under key2, and section2 has no keys.

Have you added test cases to catch the problem?

Yes, I've added tests that cover the issue and fail on the current tip of main.

assert.Equal(t, "value3", f.Section("section2").Key("key3").String())
})

t.Run("no nested values and following sections with indentation", func(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only test that fails prior to my fix, but I added the other subtests just to have some more comprehensive coverage, since there are currently no nested value tests at all.

@unknwon unknwon changed the title Don't allow nested values to span sections Fix nested values can span sections Dec 2, 2021
Copy link
Member

@unknwon unknwon left a comment

Choose a reason for hiding this comment

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

Thank you!

@unknwon
Copy link
Member

unknwon commented Dec 2, 2021

GitHub Action got stuck and does not run, I'm gonna try close and reopen the PR see if helps.

@unknwon unknwon closed this Dec 2, 2021
@unknwon unknwon reopened this Dec 2, 2021
@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #306 (4abc582) into main (14e9811) will increase coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #306      +/-   ##
==========================================
+ Coverage   88.11%   88.21%   +0.09%     
==========================================
  Files           9        9              
  Lines        1355     1349       -6     
==========================================
- Hits         1194     1190       -4     
+ Misses         98       96       -2     
  Partials       63       63              

@unknwon unknwon merged commit c71eccd into go-ini:main Dec 2, 2021
@unknwon
Copy link
Member

unknwon commented Dec 2, 2021

https://github.com/go-ini/ini/releases/tag/v1.66.2 has been tagged for this merge.

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