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

Possible bug when using both MentionFilter and TeamMentionFilter #412

Merged

Conversation

jeremysmithco
Copy link
Contributor

I've been having trouble getting MentionFilter and TeamMentionFilter to both work together. (If only one is used, then it's correct, but when used in combination, it appears that only the second filter will be applied.)

Sorry for just sharing a failing test, but I wanted to demonstrate what I think the issue is. I've spent a few hours today trying to figure it out and I haven't found the root problem yet.

@jeremysmithco jeremysmithco changed the title Add failing test surfacing issue using MentionFilter and TeamMentionFilter Possible bug when using both MentionFilter and TeamMentionFilter Aug 4, 2024
@gjtorikian
Copy link
Owner

I've spent a few hours today trying to figure it out and I haven't found the root problem yet.

I...haven't found it either. 😆 Granted I chewed on this for about ten minutes. But it seems like the text in the TeamMentionFilter is the original text — as if the second filter is not getting the MentionFiltered text at all.

@jeremysmithco
Copy link
Contributor Author

But it seems like the text in the TeamMentionFilter is the original text — as if the second filter is not getting the MentionFiltered text at all.

Yeah! That's what I was concluding... I thought it might be IGNORE_PARENTS, but that didn't seem to be it. Then I thought maybe some state was leaking somehow, but I couldn't see what. Then I thought the problem might lie in the call to Selma::Rewriter...but I didn't get down to that level to test.

@gjtorikian
Copy link
Owner

Yes, it's almost certainly Selma related. I'm looking into it!

@gjtorikian
Copy link
Owner

This is 100% in Selma. I'd like to keep this PR open as a useful test case for when Selma is updated, but the real work is going on over there.

@jeremysmithco
Copy link
Contributor Author

Thanks Garen, much appreciated! 🙏 Will watch the progress over there. 😊

@gjtorikian
Copy link
Owner

selma 0.4.7 has fixed this!

@gjtorikian gjtorikian merged commit 3e9be8d into gjtorikian:main Aug 17, 2024
3 checks passed
@jeremysmithco jeremysmithco deleted the bug-combining-mention-filters branch August 17, 2024 16:13
@jeremysmithco
Copy link
Contributor Author

Awesome, thanks a ton @gjtorikian!

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.

2 participants