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 Range method #102

Merged
merged 4 commits into from
May 30, 2023
Merged

Add Range method #102

merged 4 commits into from
May 30, 2023

Conversation

gozeloglu
Copy link
Contributor

* This fixes jellydator#71.

* The code is not tested.

Signed-off-by: Gökhan Özeloğlu <gozeloglu@gmail.com>
@swithek
Copy link
Contributor

swithek commented Apr 30, 2023

@gozeloglu three PRs in a row, you're on fire!

As for testing, I think something like this should work:

c := prepCache("1", "2", "3", "4", "5")
var results []string
c.Range(func(item *Item) {
    results = append(results, item.Key())
    return item.Key() != "4"
})
assert.Equal(t, []string{"1", "2", "3", "4"}, results) // order is important

cache.go Outdated Show resolved Hide resolved
Signed-off-by: Gökhan Özeloğlu <gozeloglu@gmail.com>
@gozeloglu
Copy link
Contributor Author

@gozeloglu three PRs in a row, you're on fire!

As for testing, I think something like this should work:

c := prepCache("1", "2", "3", "4", "5")
var results []string
c.Range(func(item *Item) {
    results = append(results, item.Key())
    return item.Key() != "4"
})
assert.Equal(t, []string{"1", "2", "3", "4"}, results) // order is important

I added a simple test but I only checked the "5" because of map internal.

@gozeloglu gozeloglu requested a review from swithek May 4, 2023 19:33
@gozeloglu
Copy link
Contributor Author

Hi @swithek. Can you review the last changes?

@swithek
Copy link
Contributor

swithek commented May 16, 2023

@gozeloglu sorry for the delay. I'll review your changes sometime in the next few days.

@swithek
Copy link
Contributor

swithek commented May 19, 2023

I added a simple test but I only checked the "5" because of map internal.

My proposal in the comment above uses the LRU list instead of the map, so you should be able to get predictable results and check all items in the result slice. @gozeloglu

* For loop changed.

* Test case updated.

Signed-off-by: Gökhan Özeloğlu <gozeloglu@gmail.com>
@gozeloglu
Copy link
Contributor Author

I added a simple test but I only checked the "5" because of map internal.

My proposal in the comment above uses the LRU list instead of the map, so you should be able to get predictable results and check all items in the result slice. @gozeloglu

I updated the test. The keys are added to the front of the linked list. So, I checked in reverse order.

cache.go Outdated Show resolved Hide resolved
* RLock() before for loop, re-RLock inside for loop.

Signed-off-by: Gökhan Özeloğlu <gozeloglu@gmail.com>
@swithek swithek merged commit c619bbd into jellydator:master May 30, 2023
@gozeloglu gozeloglu deleted the add-range-foreach branch May 30, 2023 12:32
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.

Add Range/ForEach method
2 participants