Anomie, thanks for the review!  Inline...

API documentation is severely lacking, see
> http://en.wikipedia.beta.wmflabs.org/w/api.php?action=help&modules=editlist|query+listpages|query+lists
> <http://en.wikipedia.beta.wmflabs.org/w/api.php?action=help&modules=editlist%7Cquery+listpages%7Cquery+lists>
>

Agree, should be fixed now that API is more stable.

>
> Lots of internal FauxRequests calls to ApiMain, that's bad code structure.
>

Are you suggesting we should not use API internally?  I do think we should
have a better set of functions for calling API internally as part of the
core functionality. I wanted to start the discussion on that, but
separately from this thread.


> Database error when using lspnamespace.
>

Good catch, thanks, fixed in https://gerrit.wikimedia.org/r/#/c/199615/


>
> Ordering/continuation in ApiQueryListPages::queryListItems isn't
> necessarily total, if the DB somehow gets duplicate gli_order.
>

Yep, patch was already pending for that -
https://gerrit.wikimedia.org/r/#/c/199510/


> ApiQueryListPages should be putting its result array under the 'query'
> node, not as its own top-level node. You probably copied list=watchlistraw,
> for which see T36356.
>

Thanks, will fix


>
> Continuation is broken here:
> http://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&list=lists&lstids=51|57&lstlimit=1
> (your continuation value should include all fields in your ORDER BY)
>
> There's a similar possibility for broken continuation when 'mode' is set,
> or else your query is probably making MySQL be stupid by ordering by
> constant gl_perm.
>

gl_perm is a constant for that query, i was unsure if Mysql would pick the
right index here, so included it in the "order by" just to be safe.  I
could force index instead - what's the best practice with the new mysql?


>
> You're returning booleans as booleans without code for adding the metadata
> being added in Gerrit change 182858, which means you're going to have
> issues once that change is merged. You'll probably also have some
> deprecation warnings once that is merged.
>

Will look at it, thx for the heads up.

>
>
> Bad tag names in
> http://en.wikipedia.beta.wmflabs.org/w/api.php?action=query&list=lists&format=xmlfm&lstprop=count,
> because you're directly screwing up the metadata element.
>
>
Thanks, will fix.
_______________________________________________
Mobile-l mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mobile-l

Reply via email to