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

MentionFilter#link_to_mentioned_user: Replace String introspection with Regexp match #172

Merged
merged 1 commit into from
Jan 21, 2015

Conversation

simeonwillbanks
Copy link
Contributor

#167 introduced a subtle bug by relying upon ActiveSupport's String#last. The tests passed because the test helper requires active_support/core_ext/string.

Not all applications will require ActiveSupport's String, and the extension seemed heavy for the MentionFilter.

This Pull Request improves the existing Regexp, so the MentionFilter doesn't need ActiveSupport's String.

Commit Message:

  • last requires ActiveSupport and applications may not have required
    'active_support/core_ext/string'
  • Instead of extracting the last character and matching against that
    character, refactor the Regexp, so it tests the last character of
    the base_url

- `last` requires ActiveSupport and applications may not have required
   'active_support/core_ext/string'
- Instead of extracting the last character and matching against that
  character, refactor the Regexp, so it tests the last character of
  the `base_url`
@JuanitoFatas
Copy link
Contributor

@simeonwillbanks thank you! 👍

jch added a commit that referenced this pull request Jan 21, 2015
…rt-bug

MentionFilter#link_to_mentioned_user: Replace String introspection with Regexp match
@jch jch merged commit 0e9be63 into master Jan 21, 2015
@jch
Copy link
Contributor

jch commented Jan 21, 2015

🍻

@jch jch deleted the mention-filter-requires-active-support-bug branch January 21, 2015 22:44
@simeonwillbanks
Copy link
Contributor Author

😄 Thanks everyone!

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.

3 participants