Hey @iperzic. I always disliked how smart filtering behaved.
I've pushed some more fixes on top of yours - let me know what you think.

This is how I want it to behave, when toggling smart filtering:

• if we're filtering by the current repo, remove that filter
• if we're filtering by another repo - do nothing
• if we're not filtering by any repo - add the current repo as filter
• auto add the current repo as filter on launch if it's missing and smartFilteringAtLaunch is set to true (which is the default).

Let me know if my fixes make sense and I would love for more tests are that.
I kept your fix working, just broken it into a function.

CC @sideshowbarker

There’s certainly always been a huge amount of room for improvement in the smart-filtering feature. So it’s great to see it getting some attention and fixes/refinements.

sideshowbarker@2112c16 has some further fixes, plus tests.

I don’t have push/write perms to this repo, nor to PR branches here like this one — so that commit is at my own repo. But it could be fetched and cherry-picked into this branch.

@dlvhdr Thanks for the review, and the follow-up. I included @sideshowbarker commit in this PR as well.

As far as I'm concerned, this looks good. Though, I must caveat that I'm not that experienced with golang, so take my review with a bucket of salt.

@sideshowbarker seems like the tests fail due to not running inside a git repo or something along those lines. Should we mock the currentRepoFilter func?

@sideshowbarker seems like the tests fail due to not running inside a git repo or something along those lines.

To fix that, I see you made this change:

 func currentRepoFilter(t *testing.T) string {
 	t.Helper()
+	t.Setenv("GH_REPO", "https://github.com/dlvhdr/gh-dash")
 	repo, err := repository.Current()
 	if err != nil {
 		t.Fatal("failed to resolve current repository:", err)

So yeah I hadn’t considered how that CI might not (wouldn’t) have a remote configured.

But in hindsight: I can see that, because the test as currently written isn’t testing across multiple repos anyway, then for the current scope of the test, the test didn’t need currentRepoFilter to begin with. We could just do this:

const testRepoFilter = "repo:dlvhdr/gh-dash"

…and then just have the test use that constant everywhere it’s calling currentRepoFilter now.

Should we mock the currentRepoFilter func?

No, I think the fix you made is sufficient for now. The actual app code doesn’t call currentRepoFilter anywhere. It’s just a test-only thing.

I had imagined the test needed to handle testing across multiple repos, and so we’d need some way for the test to know what the current repo is. But… the test as currently written isn’t testing across multiple repos — and even if it were, we could anyway actually just be using strings.

So when (if) we next have need to go back in and touch that section_test.go test, then we should refactor it to replace the currentRepoFilter function and calls.

But otherwise, I think the fix you made is fine for now, and we don’t need to fiddle with it further right now.