Return an empty array from `ValueWithShadows` if there is none #313

NDagestad · 2022-01-12T18:43:04Z

When there are no value for a given key, ValueWithShadows used to return an array containing an empty string. Instead, this PR makes it return an empty array. This change produces the same behaviour for StringsWithShadows as it uses ValueWithShadows to do it's job.

Link to the issue: #310

Checklist

I agree to follow the Code of Conduct by submitting this pull request.
I have read and acknowledge the Contributing guide.
I have added test cases to cover the new code.
There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

PS: I tried to wait a little before doing the PR in case any newcomer wanted to use the opportunity of this simple fix for their first PR, but it's been a month now so I'll just do it myself ¯\_(ツ)_/¯

unknwon · 2022-01-13T05:46:54Z

There aren't any existing test cases for ValueWithShadows. Should I add a new one ?

Yes, please do so!

With the previous behaviour, many tests broke because it was not able to distinguish between a key that was present but had no value and a key that was not present at all.

NDagestad · 2022-01-15T15:45:32Z

It was wise of you to make me do tests, I found out that my original way of doing it broke a few things that I did not see because it had no effect on my use case. I also edited the .editorconfig to not trim trailing spaces in test files since it got me when I first saved the file after adding my test 🙃

unknwon

Thanks for the follow up!

.editorconfig

codecov · 2022-01-17T00:57:03Z

Codecov Report

Merging #313 (3b04438) into main (c71eccd) will decrease coverage by 0.25%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main     #313      +/-   ##
==========================================
- Coverage   88.21%   87.95%   -0.26%     
==========================================
  Files           9        9              
  Lines        1349     1362      +13     
==========================================
+ Hits         1190     1198       +8     
- Misses         96       98       +2     
- Partials       63       66       +3

NDagestad · 2022-01-17T10:58:46Z

In TestKey_NestedValues, the s3 key has a trailling space after the equal sign in the expected output in the assertion.

NDagestad · 2022-01-17T13:22:24Z

Ha! I forgot to give the test case a description, and it seems that it is not ran, did I miss something ? (Haven't done tests in go before so it is not unlikely I did something wrong)
EDIT: Yes I did, the function should start with an upper case "t", and I used a lower case one... I'll fix it later when I get home

File.WriteToBuffer also needed to be tweaked so it could work with keys that are present but have no value.

NDagestad · 2022-01-17T19:11:20Z

Third time's the charm !
This time I have checked that my test is being run, and everything works as it should.
It turns out that my seccond attempt was flawed but the first method was working as intended.
If the Key is produced by the section.Key() function, using section.HasKey() will not work since it has been
added to the section in Section.Key(). Simply deciding to return an empty array based on the
content of Key.Value does work though, but File.WriteToBuffer() loops on the output
Key.ValueWithShadow() so if an empty array is returned, that key will not be added to the output
even if it was in the input but had no value (I assume this is a behaviour that you want to keep, and I
would agree.)
So I had to add a special case to be handled before the loop in case an empty array was returned.

unknwon · 2022-01-18T12:53:28Z

file.go

+				if _, err := buf.WriteString(kname); err != nil {
+					return nil, err
+				}
+				// Write out alignment spaces before "=" sign
+				if PrettyFormat {
+					buf.Write(alignSpaces[:alignLength-len(kname)])
+				}
+				if _, err := buf.WriteString(equalSign + LineBreak); err != nil {
+					return nil, err
+				}
+			}
+
+			for _, val := range shadows {


I think this block is missing many handlings from where it is copied from (below).

We should either make this loop (starts on line 459) body to be an inline function .e.g writeKey := func(k *Key) error {...}, or do something like:

keys := make([]*Key, 1, len(shadows)+1) keys[0] = key keys = append(keys, shadows) for _, val := range keys { ...

But the latter is obviously more expensive IMO (due to allocations).

I do not see what handlings are missing, the value check don't have any meaning since it is in a case where the key has no associated value, and I imagine a boolean key has a value so it wouldn't pass the condition and be handled in the loop like before, am I missing something ?
Anyways I am not a fan of the duplicated code like I did, I do prefer the first solution you proposed (partly because I don't really get how the second one would work) do you really prefer the function to be inline ? If so why ? (Not that I really have anything against it, I am just curious)

Also, I see codecov is not happy with my patches, is it something I should care about ? (go test -cover gives a different code coverage value from codecov)

I do not see what handlings are missing, the value check don't have any meaning since it is in a case where the key has no associated value, and I imagine a boolean key has a value so it wouldn't pass the condition and be handled in the loop like before, am I missing something ?

Ah, you're right. So alternatively, you could add a code commend explaining why we don't need to deal with values here.

But since we can factor out a function, I think having a unified logic with an inline function is relatively more robust (if anything changes to the key name handling).

(partly because I don't really get how the second one would work)

Hmm, that was for illustration mostly, on a second look I also forgot how it should work 😅 so forgot about it.

do you really prefer the function to be inline ? If so why ? (Not that I really have anything against it, I am just curious)

Yes, because I don't a reason why it shouldn't be inlined. It is only used here and never meant to be used outside, and the function body is also pretty sophisticated.

Also, I see codecov is not happy with my patches, is it something I should care about ? (go test -cover gives a different code coverage value from codecov)

Yeah... the difference is annoying, CodeCov is more FYI, don't need to worry much unless diff coverage is 0 🤫.

…cation in WriteToBuffer

NDagestad · 2022-01-19T19:00:29Z

I was a bit on the fence about passing only the value to write in the function and using key and kname from the calling function, but it would ba a bit redundant to pass them as parameters and kname is realistically never going to change since the function is declared for the current key (don't know if what I said really made any sense)

file.go

unknwon · 2022-01-20T08:38:29Z

Thank you, merging!

unknwon · 2022-01-20T08:39:31Z

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

Return an empty array from ValueWithShadows if there are none

0e2a128

NDagestad added 3 commits January 15, 2022 16:40

Change the way of deciding if a value should be returned or not

6ed3295

With the previous behaviour, many tests broke because it was not able to distinguish between a key that was present but had no value and a key that was not present at all.

Trailing whitespaces should not be trimed in test files

ce6c1ab

Add tests for ValueWithShadows

9fca1cd

unknwon requested changes Jan 17, 2022

View reviewed changes

.editorconfig Show resolved Hide resolved

revert to the original decision behaviour for ValueWithShadows

12524e3

File.WriteToBuffer also needed to be tweaked so it could work with keys that are present but have no value.

unknwon self-requested a review January 18, 2022 04:06

unknwon changed the title ~~Return an empty array from ValueWithShadows if there are none~~ Return an empty array from ValueWithShadows if there is none Jan 18, 2022

unknwon changed the title ~~Return an empty array from ValueWithShadows if there is none~~ Return an empty array from ValueWithShadows if there is none Jan 18, 2022

unknwon requested changes Jan 18, 2022

View reviewed changes

Extract the value writting logic to an function to prevent code dupli…

d77100b

…cation in WriteToBuffer

unknwon approved these changes Jan 20, 2022

View reviewed changes

file.go Outdated Show resolved Hide resolved

Update file.go

3b04438

unknwon merged commit fcd6cc3 into go-ini:main Jan 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return an empty array from `ValueWithShadows` if there is none #313

Return an empty array from `ValueWithShadows` if there is none #313

NDagestad commented Jan 12, 2022 •

edited by unknwon

Loading

unknwon commented Jan 13, 2022

NDagestad commented Jan 15, 2022

unknwon left a comment

codecov bot commented Jan 17, 2022 •

edited

Loading

NDagestad commented Jan 17, 2022 via email

NDagestad commented Jan 17, 2022 •

edited

Loading

NDagestad commented Jan 17, 2022

unknwon Jan 18, 2022

NDagestad Jan 18, 2022

NDagestad Jan 18, 2022

unknwon Jan 19, 2022 •

edited

Loading

unknwon Jan 19, 2022 •

edited

Loading

NDagestad commented Jan 19, 2022

unknwon commented Jan 20, 2022

unknwon commented Jan 20, 2022

Return an empty array from ValueWithShadows if there is none #313

Return an empty array from ValueWithShadows if there is none #313

Conversation

NDagestad commented Jan 12, 2022 • edited by unknwon Loading

Checklist

unknwon commented Jan 13, 2022

NDagestad commented Jan 15, 2022

unknwon left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 17, 2022 • edited Loading

Codecov Report

NDagestad commented Jan 17, 2022 via email

NDagestad commented Jan 17, 2022 • edited Loading

NDagestad commented Jan 17, 2022

unknwon Jan 18, 2022

Choose a reason for hiding this comment

NDagestad Jan 18, 2022

Choose a reason for hiding this comment

NDagestad Jan 18, 2022

Choose a reason for hiding this comment

unknwon Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

unknwon Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

NDagestad commented Jan 19, 2022

unknwon commented Jan 20, 2022

unknwon commented Jan 20, 2022

Return an empty array from `ValueWithShadows` if there is none #313

Return an empty array from `ValueWithShadows` if there is none #313

NDagestad commented Jan 12, 2022 •

edited by unknwon

Loading

codecov bot commented Jan 17, 2022 •

edited

Loading

NDagestad commented Jan 17, 2022 •

edited

Loading

unknwon Jan 19, 2022 •

edited

Loading

unknwon Jan 19, 2022 •

edited

Loading