MediaWiki r64726 - Code Review

Jump to: navigation, search
Revision:r64725‎ | r64726 (on ViewVC)‎ | r64727 >
Date:00:29, 8 April 2010
Status:ok (Comments)
Bug 22474, urlencode now takes a parameter to change escaping style
Modified paths:

Diff [purge]

Loading diff…


#Comment by Werdna (talk | contribs)   03:44, 9 December 2010

Seems OK to me, but I'm not familiar enough with magic words to mark as ok.

#Comment by MarkAHershberger (talk | contribs)   17:25, 10 December 2010

+ case 'url_query':

+			default:

Looks good, but I would add another magic word test, say |UNKNOWN so that this could be tested in cases where it is changed.

#Comment by Conrad.Irwin (talk | contribs)   20:01, 10 December 2010

The point of the default is not to provide behaviour for "UNKNOWN" — just to replicate the behaviour when there is no parameter specified, which is tested. If you think that passing an unknown parameter should always work as though no parameter is passed, then maybe it needs testing too. I would prefer to leave that behaviour undefined until it's clear that showing an error message is the wrong thing to do.

#Comment by MarkAHershberger (talk | contribs)   20:25, 10 December 2010

If an unknown parameter is specified, and you think the proper thing to do is return an error, then you should do that and test for this behavior.

#Comment by Conrad.Irwin (talk | contribs)   20:36, 10 December 2010

I don't know what the proper behaviour is — that's why I didn't test for it :p. I'm not too fussed either way, on the one hand I like to encourage people to use the right syntax, on the other, it's pretty unfriendly if it "just breaks".

#Comment by MarkAHershberger (talk | contribs)   23:10, 10 December 2010

I understand, My intention was to suggest that the tests reflect the current behavior (error or no) so that the software does not change behavior unexpectedly.

Status & tagging log