@mention allow for custom regex to identify usernames.

[brittballard]

• This allows for easier customization of the mention patter regex

[brittballard]

@jch I believe this is the modification discussed in the previous PR.

[jch]

@brittballard while this does allow a user to overload the MentionPattern regex, it still requires a user to subclass the filter in order to change it. A better implementation would be to have an :mention_pattern that's available in the context. For example, :base_url and :info_url are context variables. (https://github.com/brittballard/html-pipeline/blob/mentions-starting-with-underscore/lib/html/pipeline/%40mention_filter.rb#L9-L13)

Care to give that implementation a go?

[brittballard]

Ah, yes that looks very nice. Sure I'll resubmit. Thank you.

[brittballard]

@jch what do you think of this? I set it up to only allow for modification of the username to avoid confusion. Allowing someone to change the entire regex was problematic because the rest of the class depends on the presence of the "@", which makes sense to me.

[jch]

Downside to this is that a new regex object is created everytime the filter
is run. I do like your idea of only specifying the username pattern so
callers don't have to worry about trailing white space or the leading @.
Maybe only create a fresh pattern if a username pattern was specified in
the context?

On Monday, October 6, 2014, Britt Ballard [email protected] wrote:

@jch https://github.com/jch what do you think of this?

—
Reply to this email directly or view it on GitHub
#157 (comment).

Jerry Cheung
GitHub https://github.com/jch

[brittballard]

@jch that's a good point. Sure I'll look into taking that route.

[brittballard]

@jch what do you think of this solution?

[brittballard]

Working on that bad test now.

[jch]

This is a simple and clever solution. 👍

Are the tests passing for you? The Hash#new docs suggest that you're responsible for storing the value. I haven't tried running it yet though.

It is the block’s responsibility to store the value in the hash if required.

[brittballard]

The test do currently pass. I read that as telling me I had to include hash[key] in the block (line 46). If there is a simpler or cleaner way I'm all for it.

[jch]

👍

[jch]

This test seems too tied to the implementation to me. It's testing the definition of Hash.new rather than actual behavior. I think we can do without it. I think you can roll this test into the one below, and assert that filtering with the default regex leads to a pattern_count of 1, and filtering again with a custom regex gives a pattern_count of 2.

[brittballard]

Cool, I'll make the change.

[jch]

@brittballard it's coming together!

I noticed you were doing force pushes for the changes, which is fine if you prefer it. But I don't have any problems with having the intermediate commits either. Your call.

[brittballard]

@jch I have gotten into the dangerous habit of using --amend when I'm working on my own branches. It has bitten me a time or two, I probably need to get back to not doing it!

[jch]

@brittballard I like using amend also, but limit myself to only commits that haven't been pushed up yet. git rebase -i is also nice for tidying up history before a public push.

[brittballard]

That's a good policy.

[jch]

Ready for review again, or were there other changes you wanted to make?

[brittballard]

Ready for review, sir.

[jch]

👍 slated for 2.0 release

🍻