Lullabot / lubot

1 stars 0 forks source link

regex tidy up :) #48

Closed willwh closed 9 years ago

willwh commented 9 years ago

@m4olivei I would love some regex advice :)

https://github.com/Lullabot/lubot/blob/drupalize.me/lubot.js#L168

https://github.com/Lullabot/lubot/blob/drupalize.me/lubot.js#L176

https://regex101.com/r/pG5uB3/1

Ideally, we would not be matching on say; +++++++++++, but only instances of ++

q0rban commented 9 years ago

I think if someone does +++++++, you still match it, but strip the extra +es. So, you'd just do \+{2,}, right?

m4olivei commented 9 years ago

I agree with @q0rban, not sure it matters to limit the number of "+"'s used. In which case the regex is good, will match correctly and capture the username. I noticed some slack username, (eg. t-bot) have dashes, so regex would miss those. Also a couple other cleanup:

https://regex101.com/r/pG5uB3/4

If you wanted to match only two "+"s, here's one that could work. It works by requiring a character that isn't a "+" after the first two, or the end of line position:

https://regex101.com/r/pG5uB3/2

q0rban commented 9 years ago

Good catch on the -. I think that means we'd also want other characters too, like underscores and … what else?

q0rban commented 9 years ago

I think there are some other scenarios we need to account for:

something nifty++
something_nifty++
tlattimore|afk++
[tlattimore]++
this shouldn't be a match for willwh++
we don't want $i++ to increment a karma value for i
thisiswaytoolongandshouldnotgetmatched++
m4olivei commented 9 years ago

Here's an error message I get when I try to change my name so something with all kinds of characters:

image

I'll make a PR to correct some of these regex's. Will need getting my local setup for testing though.

q0rban commented 9 years ago

Remember, you can give karma to things other than just people:

[12:02:53]  <q0rban>    ie6--
[12:02:54]  <lullabot>  ie6 has karma of -22.
m4olivei commented 9 years ago

Right, and now that I read through the whole diff, seems like you were thinking about just users @willwh ?

m4olivei commented 9 years ago

Tried something that makes sense in my head in a5b85a2. Need to test though and I can't get the node server to start locally on this branch anymore :/.

willwh commented 9 years ago

I pushed some updates specific to my setup, so it doesn't even allow http connections. I'll hook up with you this afternoon and I can push something you can work with.

That Drupalize.me branch is dirty ATM ;) On 22 May 2015 09:38, "Matthew Oliveira" notifications@github.com wrote:

Tried something that makes sense in my head in a5b85a2 https://github.com/Lullabot/lubot/commit/a5b85a2f838ce37831a5da98134ae88928596039. Need to test though and I can't get the node server to start locally on this branch anymore :/.

— Reply to this email directly or view it on GitHub https://github.com/Lullabot/lubot/issues/48#issuecomment-104707572.

m4olivei commented 9 years ago

Cool, ping me whenever.

m4olivei commented 9 years ago

Calling this guy closed. Going to open more specific issues as they come.