Wikipedia:Code review/PC bot

PC bot

The following discussion is closed. Please do not modify it. Subsequent comments should be made on the appropriate discussion page. No further edits should be made to this discussion.

Code review of: PC bot.js
Review requested by DannyS712 at 06:59, 7 April 2019 (UTC)[reply]

Current code: User:DannyS712 test/PC bot.js

Specific issues / areas for feedback

This isn't a request to review potential changes, but more like a request to review past changes. I recently tried to convert this script to run asynchronously (https://en.wikipedia.org/w/index.php?title=User%3ADannyS712_test%2FPC_bot.js&type=revision&diff=891178202&oldid=891174776) so that categories would be processed one at a time (this script goes through different polluted categories and removes user pages, for Wikipedia:Bots/Requests for approval/DannyS712 bot 11).

I know next-to-nothing about javascript promises, deferred objects, 'await's, etc, and was hoping that someone could tell me if this was the right way to implement my goal (it works, but is it a good way?) or if there is a better method to use.

Thanks, DannyS712

Discussion

@DannyS712: Well, far be it for me to claim any particular expertise here, but your async/await approach does indeed look fine. For a slightly more involved application I might have investigated some kind of workqueue system, but for the purpose your approach seems eminently suited.

Apart from that I noticed a couple of other things while looking at the code. First, it's using importScript() which is deprecated. It's also loading page.js on every page, not just on the reports page. And that nested $(document).ready() is weird and probably doesn't work quite like the code assumes it works. I took a stab at rewriting that bit (untested):

mw.loader.using('mediawiki.util', function() {
  $(document).ready(function() {
    if (mw.config.get('wgPageName') === "Wikipedia:Database_reports/Polluted_categories") {
      mw.loader.getScript('/w/index.php?title=User:DannyS712 test/page.js&action=raw&ctype=text/javascript')
        .then(function() {
          let caction = mw.util.addPortletLink(
            'p-cactions', 'javascript:void(0)', 'Polluted categories', 'ca-polluted', 'TOOLTIP'
          );
          $(caction).click(function(e) {
            e.preventDefault();
            Polluted_categories();
          });
        }, function (e) {
            // Script load failed.
            mw.log.error(e.message);
        });
      });
    }
  );
});

This moves importing page.js inside the if statement so it only loads on the relevant page. It also uses mw.loader.getscript() to load it, and since this returns a Promise object you can chain a .then() on it that won't run until the promise is resolved (page.js finishes loading). .then() takes two functions as arguments: the first one runs on successful completion, and the second if the Promised action fails (you can just drop the second argument for cleaner code if you don't care about load errors here). I also switched to the shortcut .click() and threw on a e.preventDefault() on general principle. --Xover (talk) 16:41, 15 May 2019 (UTC)[reply]

Oh, and I think some of the pain in the async/await bits there stem from using jQuery's $.get(), which is based on callbacks, instead of the mw.api methods, that are Promise-based. I haven't really looked at mw.api so I'm not sure how involved switching over will be, but it would probably make for a better model for this than async/await (which are… really weird). --Xover (talk) 16:59, 15 May 2019 (UTC)[reply]

Thanks for the follow up, sorry I didn't see this. I'll test your suggested changes when I get a chance. Thanks, --DannyS712 (talk) 11:37, 30 June 2019 (UTC)[reply]

might be a bit late but still... @DannyS712: Is speed important in this application? If yes, using await makes the process a lot slower than it can be. Implementing a workqueue-like system will require a lot of voo-doo but there is a simpler way, if you are willing to add a js dependency (use mw.loader.getScript('scripturl?action=raw&ctype=text/javascript').using(callback)), See Mediawiki:Gadget-morebits.js's Morebits.batchOperation class. It is used in Twinkle's unlink, batchdelete, batchundelete, etc and it works fast. Basically what it does is take an array of pages, split it into chunks of 50 (count customisable) and send the API requests to the server for dealing with 50 pages at the same time - which is not too many to clog the server and cause the requests to fail, nor too less to take up a lot of time. The status message for each page can also be seen by running Morebits.status.init(some-div-element-to-show-statuses-in) prior to executing the batch operation. SD0001 (talk) 17:54, 26 June 2019 (UTC)[reply]

@SD0001: Not too late, thanks for the feedback. I'll look into the idea, though speed isn't very important --DannyS712 (talk) 11:37, 30 June 2019 (UTC)[reply]

The discussion above is closed. Please do not modify it. Subsequent comments should be made on the appropriate discussion page. No further edits should be made to this discussion.