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

space separated callouts break middle callouts #257

Closed
btbonval opened this issue Mar 23, 2015 · 7 comments

Comments

Projects
None yet
2 participants
@btbonval
Copy link
Member

commented Mar 23, 2015

followup from @jywarren 's report here #116 (comment)

Can be exemplified here:
http://rubular.com/r/Vevai8HVFi

In this case, @mathew @natalie @becki sees @mathew grabbing the space after the callout, which prevents @natalie from being seen for some reason.

@btbonval

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

Changing the list to comma separated works fine. Not that we should require it, just saying.

@btbonval

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

Yeah the pattern requires either start of line or a non-alphanumeric prior to @, this ensures we aren't looking at an email.

Because @mathew steals the non-alphanumeric prior to @natalie, it cannot be matched. Unfortunately, @natalie, while having nothing in the front and seemingly a match for ^, does not actually match because it is not the beginning of a line.

I wonder if there's a way to say "nothing" rather than ^.

EDIT: start of string \A seems like a good replacement for start of line ^. It appears to perform exactly the same way. Replacing end of string \z for end of line $ also seems better in the long run, but makes no difference for this problem.

@btbonval

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

Removing that last (\W) group (be it combined with $ or \z) seems to fix the problem. However I'd like to know how that ended up there in the first place. Was it just overthinking the problem or is there a reason for it?
http://rubular.com/r/1abUdK8oZf

@btbonval

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

I reviewed when the current regex was written. The final non-alphanumeric group was added by me using the example in the first paragraph of the above rubular link. Removing that final non-alphanumeric group still works just fine on that first paragraph, so it was probably excessive.

I think we can remove that final group and solve the problem. @jywarren If you're willing to gamble on my being correct, I'll just remove that final group from the regex directly in the file on github. The regex has been tested in rubular, so I probably don't need to fire up the whole website to test it, right?

@btbonval

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2015

Remove the final group (\W|$) from this line:

FINDER = /(^|\W)\@([\w-]+)(\W|$)/

Remove references to the final group \3 in both these lines. These were used to inject a trailing period, space, or whatever was matched back into the output, but now that we don't match it, it shouldn't need to be inserted back in.

PRETTYLINKMD = '\1[@\2](/profile/\2)\3'
PRETTYLINKHTML = '\1<a href="/profile/\2">@\2</a>\3'

@jywarren jywarren added the bug label Apr 1, 2015

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

I've written a test for this case, and can try the fix against it.

jywarren referenced this issue in jywarren/plots2 Oct 15, 2015

@jywarren jywarren self-assigned this Oct 15, 2015

@jywarren jywarren closed this Oct 15, 2015

@jywarren jywarren removed the in-progress label Oct 15, 2015

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

tested and merged!

jywarren referenced this issue in jywarren/plots2 Oct 15, 2015

jywarren added a commit that referenced this issue Oct 15, 2015

Merge pull request #330 from jywarren/callout-fixes
fixes #257 and creates matching tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.