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

Add option that causes nil slices to equal empty slices. #27

Merged
merged 5 commits into from
Jan 16, 2020

Conversation

Anaminus
Copy link
Contributor

@Anaminus Anaminus commented Apr 7, 2019

Often times when testing, the difference between a slice that is nil and a slice that contains zero elements isn't useful. This change adds the NilSlicesAreEmpty variable, which is initially false to retain backward compatibility. When set to true, slices are compared by length rather than by nilness. Care has been taken to ensure that the resulting diff still indicates that a slice is nil versus empty.

Often times when testing, the difference between a slice that is nil
and a slice that contains zero elements isn't useful. This change adds
the NilSlicesAreEmpty variable, which is initially false to retain
backward compatibility. When set to true, slices are compared by length
rather than by nilness. Care has been taken to ensure that the
resulting diff still indicates that a slice is nil versus empty.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 254cd47 on Anaminus:nil-slices-are-empty into 042da05 on go-test:master.

@daniel-nichter
Copy link
Member

I'm ok with the option, but can you refactor the logic a little to make it easier to follow? I think in original code, the block on line 276 that starts with if a.IsNil() || b.IsNil() {should be kept and this new logic put in there. Reason being: that block is all the logic for one or the other slice being nil. And the return on 282 implicitly means "both are nil" (there should be a code comment for that).

For tests, TestNilSlicesAreEmpty is great, thank you. Let's also make a new test case for

a = a[:1]
b = b[:0]

because TestSlice() is getting too big and because I didn't code comment it enough, it's not clear any longer what all it's testing.

@Anaminus Anaminus closed this Jul 20, 2019
@Anaminus Anaminus deleted the nil-slices-are-empty branch July 20, 2019 11:05
@Anaminus Anaminus reopened this Jul 20, 2019
@Anaminus
Copy link
Contributor Author

Note: In a stunning turn of events, git was used poorly.

The additional logic cannot be contained within the block as you suggest. As expressed in the PR, the primary branching point must occur at NilSlicesAreEmpty.

When NilSlicesAreEmpty is true, the code must compare emptiness instead of nil-ness. The expression a.IsNil() || b.IsNil() blocks the case where either slice is non-nil but empty, so it is insufficient for comparing emptiness. This check is only useful after determining that NilSlicesAreEmpty is false.

I think the only revision that needs to be made here is to move the aLen and bLen declarations back to their original positions. Caching the lengths a little earlier doesn't really save anything, and introduces branches where they are declared, but unused.

As for the tests, I've moved the additional cases in TestSlice to a separate TestEmptySlice function. All additional cases have also been commented to make them easier to distinguish.

@ktnyt
Copy link

ktnyt commented Jan 16, 2020

Hello, has there been any progress on this PR? I've just started using this library and stumbled across a case where I would like an empty slice to equal nil. I can just implement an Equal method but I do believe some people would benefit from having this feature.

@daniel-nichter
Copy link
Member

Yeah, just need to fix the merge conflict and double check the tests. Will do that and cut a new release soon.

@daniel-nichter daniel-nichter merged commit d94f021 into go-test:master Jan 16, 2020
daniel-nichter added a commit that referenced this pull request Jan 16, 2020
@daniel-nichter
Copy link
Member

Released as v1.0.5

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

Successfully merging this pull request may close these issues.

None yet

4 participants