adicu / Courses

A course management application for Columbia.
courses.adicu.com
Other
16 stars 12 forks source link

Fix regex for spaces and explain better #169

Closed brishin closed 10 years ago

brishin commented 10 years ago

Closes #166, #167 Test cases: screen shot 2014-04-13 at 19 53 08 (The last number case was not addressed because I believe that it's relatively small.)

@natebrennand Can't do that /// regex literal stuff in vanilla JS!

brishin commented 10 years ago

Hmm. That the branches were created on origin was a mistake. Should have been on my brishin remote, sorry about that.

znewman01 commented 10 years ago

wait—what's with the last test case?

brishin commented 10 years ago

Sometimes, instead of W or BC for the faculty code, the number 8 will be used??? Anyway, I don't think this is a common use case.

znewman01 commented 10 years ago

how about ^([A-Z]{4})\s?(?:8|[A-Z]{0,2})(\d{1,4})? it matches the POLS one without breaking anything else.

brishin commented 10 years ago

I think then it would break POLS G8471, correct? (Because it'd capture 471 in the second group)

I think what I have now is fine for the poli sci example that I have because it would start searching POLS8100* which would match.

I'm also not sure if 8 is the only number that can be in that field.

znewman01 commented 10 years ago

err..yeah, you're right. crap.

why is the last group {1,4}?

brishin commented 10 years ago

Because the course numbers are actually max 4 digits long. It's just all this faculty type as a number small case that's throwing things off. But like I said, since we attach a wildcard to the end of the course number, I think it should be fine. (I was thinking of lookback, but apparently JS doesn't support that?)

znewman01 commented 10 years ago

are they ever <4 digits long? we could fix easily by switching {1,4} to {4}

brishin commented 10 years ago

This is also used to support the use case that you're searching for all the COMS 3000's courses, so if you just type COMS 3 then we translate that to COMS 3* in our actual query.

znewman01 commented 10 years ago

okay in that case i think we can just merge this