Code review comment for lp:~coalwater/lernid/fix-810122

Revision history for this message
Mohammad AbuShady (coalwater) wrote :

> While we're at it, do you think there's anything wrong with ignoring the case
> in the ircwidget, too? Seems like we might as well be consistent in this case
> unless you can think of a reason that it should be different.
I think we don't really need to ignore case in either cases, because we aren't using letter ranges like [a-z], I'm using '\w' which matches [a-z][A-Z] and underscores, so it's not really needed.

> I missed something when reviewing your fix above. The one in Browser.py
> doesn't include in the returning URL any surrounding brackets. Those
> surrounding brackets are used to tell lernid not to follow a specific URL,
> that the instructor just wanted to mention the URL for the student's future
> reference. If we were to use your regex as is we'd regress the fix for LP:
> #518229 Michael Budde applied. I'm sorry I missed that before.
Ok i see now what you're talking about, could you explain to me exactly the arguments and the return of the cleanurl function, cause if i understand it as a whole i'll be able to think better, i see i could just add the bracket at the start and the end but i still want to understand the function and what it should do.

> It would save me some work if you could base your change on
> lp:~jsjgruber/lernid/lernid-proposed
Ok so I'm going to branch this and try to re-apply my changes, if there are other merges that have the same problem just mention it in it's review and I'll redo it in the proposed branch.

> If you do it the former way you can take a look at
> how the changelog and the merges (including your other ones) look so far. You
> could also try it out and look for errors I've made (hint, hint).
Ok, I could take a look over there heh.

> Again, sorry for missing the [URL] thingy.
heh, no worries.

> Thanks again for working on this.
my pleasure :D

« Back to merge proposal