Issues/97 append non destructive #101
Conversation
|
Oops, I must have a discrepancy somewhere, sorry about that. I'll make the necessary changes. |
|
The failing tests say that empty slice and nil are "not equal". This is a great philosophical debate, I'll check if the distinction is meaningful here. |
Codecov Report
@@ Coverage Diff @@
## master #101 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 7
Lines ? 1019
Branches ? 0
=======================================
Hits ? 1019
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Deleplace)
functions/append.go, line 7 at r2 (raw file):
// It is acceptable to provide zero arguments. func (ss SliceType) Append(elements ...ElementType) (result SliceType) { return append(append(result, ss...), elements...)
Someone looking at the this for the first time may not know why it was done this way (and be tempted to undo it). Please put a comment that links the issue.
pie/append_test.go, line 9 at r2 (raw file):
// Make sure that Append never alters the receiver, or other // slices sharing the same memory, unlike the built-in append. func TestAppendNonDestructive(t *testing.T) {
I don't want to put tests in this package. Also, I would rather test the functions generated from the templates and not the templates themselves. You can move this to strings_test.go.
pie/carpointers_test.go, line 340 at r2 (raw file):
assert.Equal(t, len((carPointers)(nil).Append()), 0,
Is this trying to get around the result now being non-nil? It's better to put a check in Append() and avoid the allocation if it's not needed.
Same thing for the other cases of this.
No, this is intentional. In go there is no meaningful difference between nil slice and empty slice, so it would be a mistake to have rigid tests on a too strict equality check. This is part of why slices are not comparable in the language, even if we desire to compare for testing purpose. Allocating a slice of size zero doesn't take any memory and afaik is free (no allocation). |
Yes |
Yes |
There was a problem hiding this comment.
Reviewed 2 of 9 files at r3, 11 of 11 files at r4.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Deleplace)
There was a problem hiding this comment.
Reviewable status: 1 of 12 files reviewed, 1 unresolved discussion (waiting on @elliotchance)
pie/carpointers_test.go, line 340 at r2 (raw file):
Previously, elliotchance (Elliot Chance) wrote…
Is this trying to get around the result now being non-nil? It's better to put a check in Append() and avoid the allocation if it's not needed.
Same thing for the other cases of this.
No, this is intentional. In go there is no meaningful difference between nil slice and empty slice, so it would be a mistake to have rigid tests on a too strict equality check. This is part of why slices are not comparable in the language, even if we desire to compare for testing purpose.
Allocating a slice of size zero doesn't take any memory and afaik is free (no allocation).
There was a problem hiding this comment.
Reviewed 11 of 11 files at r5.
Reviewable status:complete! all files reviewed, all discussions resolved
See #97
This change is