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

bug in WhereIndexed #86

Closed
quexer opened this issue Oct 24, 2019 · 7 comments · Fixed by #88
Closed

bug in WhereIndexed #86

quexer opened this issue Oct 24, 2019 · 7 comments · Fixed by #88

Comments

@quexer
Copy link

quexer commented Oct 24, 2019

I believe WhereIndexed doesn't work as design:

https://play.golang.org/p/HD5-rxIkpx0

package main

import (
	"fmt"

	"github.com/ahmetb/go-linq/v3"
)

func main() {

	r := linq.Range(1, 10).
		WhereIndexed(func(i int, _ interface{}) bool {
			return i%3 == 0
		}).
		Results()
	fmt.Println(r)
}

output of the code above is
[1 2 3 4 5 6 7 8 9 10]
instead of
[3 6 9]

maybe the bug is here

https://github.com/ahmetb/go-linq/blob/master/where.go#L61

thanks!

@ahmetb
Copy link
Owner

ahmetb commented Oct 24, 2019

Hmm this looks legitimate. So you're saying this is a variable reused unintentionally?

Do you mind adding a test case for it so we can clearly see this fail. I'm also surprised we don't have such a case unit tested already.

@quexer
Copy link
Author

quexer commented Oct 25, 2019

hi, here's the test case:

I think index behavior should be the same no matter what predicate is provided. otherwise, we could not do index-based filtering with WhereIndexed

thanks

package main

import (
	"testing"

	"github.com/ahmetb/go-linq/v3"
)

var l = []string{"a", "b", "c", "d"}

// this will fail
func TestWhereIndexedWithTruePredicate(t *testing.T) {
	var idx int
	linq.From(l).WhereIndexed(func(i int, _ interface{}) bool {
		if i != idx {
			t.Errorf("i should be increased like idx in WhereIndexed with true predicate. expected(%v), got(%v)", idx, i)
		}
		idx++
		return true
	}).Results()
}

// this will pass
func TestWhereIndexedWithFalsePredicate(t *testing.T) {
	var idx int
	linq.From(l).WhereIndexed(func(i int, _ interface{}) bool {
		if i != idx {
			t.Errorf("i should be increased like idx in WhereIndexed with false predicate. expected(%v), got(%v)", idx, i)
		}
		idx++
		return false
	}).Results()
}

// suppose there's no different between ForEachIndexed and WhereIndexed as for index behavior
// this will pass
func TestForEachIndexed(t *testing.T) {
	var idx int
	linq.From(l).ForEachIndexed(func(i int, _ interface{}) {
		if i != idx {
			t.Errorf("i should be increased like idx in ForEachIndexed. expected(%v), got(%v)", idx, i)
		}
		idx++
	})
}

@ahmetb
Copy link
Owner

ahmetb commented Oct 25, 2019

hi, here's the test case:

sorry I meant to say, can you send a PR with this test case?

@ahmetb
Copy link
Owner

ahmetb commented Oct 25, 2019

func TestWhereIndexed(t *testing.T) {

for example we have this test case, you can add it next to that.
also do you have any idea why this test case passes? sorry I'm rusty a bit on this codebase.

@quexer
Copy link
Author

quexer commented Oct 26, 2019

hi @ahmetb , here's the PR:

#87

what I'm confused is all about the picture below

image

@ahmetb
Copy link
Owner

ahmetb commented Oct 26, 2019

Yeah I'm surprised the [9]int{1, 1, 1, 2, 1, 2, 3, 4, 2} test is passing but yours in #87 isn't.

The part you highlighted seems accurate. First param passed to predicate is the index of element (starting from 0) in the collection, second param is its value.

@quexer
Copy link
Author

quexer commented Oct 28, 2019

If the highlighted part is accurate, then I'm sure there's bug in WhereIndexed

kalaninja added a commit that referenced this issue Oct 28, 2019
ahmetb pushed a commit that referenced this issue Oct 28, 2019
* fixes #86

* fix example tests

* fixed formatting
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 a pull request may close this issue.

2 participants