Wikipedia:Bots/Requests for approval/Sambot 3

The following discussion is an archived debate. Please do not modify it. Subsequent comments should be made in a new section. The result of the discussion was

Approved.

Sambot 3

Operator: [[Sam Korn]] ^(smoddy)

Automatic or Manually Assisted: Automatic

Programming Language(s): PHP

Function Overview: Per request, migrate fields in {{WikiProject Films}}.

Edit period(s): Once

Already has a bot flag (Y/N): Yes

Function Details:

Get all the pages in Category:Films that need a synopsis. For each one:

Replace any of {{FilmsWikiProject}}, {{WikiProject Film}}, {{WPFILM}}, {{WP Film}} and {{FILM}} with {{WikiProject Films}}.
Migrate needs-synopsis to needs-plot
Remove obsolete parameters (|importance=, |attention=, |auto=, |nested=, |portal1-name=, |portal2-name=, |portal3-name= |portal4-name=, |portal5-name=, |article=, |start=, |end=).

The bot will only edit if an action was taken in step 2.

The code uses the Pillar framework. The source is User:Sambot/Code/Templates.php. The templates.ini file that contains the settings for the run is:

sourcetype=categorymembers
sourcename=Category:Films that need a synopsis
templatename=FilmsWikiProject:WikiProject Films,WikiProject Film:WikiProject Films,WPFILM:WikiProject Films,WP Film:WikiProject Films,FILM:WikiProject Films
fieldsremove=importance,attention,auto,nested,portal1-name,portal21-name,portal3-name,portal4-name,portal5-name
fieldsmigrate=needs-synopsis:needs-plot
onlyif=fieldsmigrate

N.B. that there is still discussion over whether to move to {{WikiProject Films}} or to continue using {{Films}}. I won't run the bot until it is clear which is the preferred option, but I'm making the request now because the change (to one line in the above config file) is incidental to the bot request.

[[Sam Korn]] ^(smoddy) 01:14, 7 March 2009 (UTC)[reply]

Discussion

I see a few issues in the code:

$settings['sourcefrom'] seems to be used but never defined.
The separatetemplate function will match any template beginning with the target; for example, when it is looking for {{WikiProject Film}} it would find {{WikiProject Filmmaking}}.
- BTW, isn't a regex of /[\D\d]*/ the same as /.*/s, or /(?s:.*)/? It's fine either way, although the latter might be more clear to others.
You need an extra \s* in your field migrator: "/(\|\s*)" . $fieldsmigratefrom[$i] . "(\s*=[^|}]*)/i". Also, as far as I can tell the part in grey is not necessary here, but shouldn't hurt anything either.
You need the same additional \s* in your field remover.
The field remover will screw up if the parameter to be removed contains nested templates: trying to remove foo from {{A|foo={{B}}}} would result in {{A|foo=}}}}.

Also, a few minor comments:

Technically, the field migration function will migrate parameters in any nested template; for example, if it was trying to migrate {{A|foo=...}} to {{A|bar=...}}, it would incorrectly change {{A|foo=...|baz={{B|foo=!!!}}}} to {{A|bar=...|baz={{B|bar=!!!}}}}. That shouldn't be a problem for this task, but could cause you trouble in the future.
The same goes for the field remover: trying to remove foo from A would change {{A|baz={{B|foo=!!!}}}} to {{A|baz={{B}}}}.

Anomie ⚔ 15:21, 7 March 2009 (UTC)[reply]

All these issues, including the minor ones, are now fixed in a major refactoring of the code -- many thanks for your comments. [[Sam Korn]] ^(smoddy) 18:27, 7 March 2009 (UTC)[reply]

There's a new bug or two:

separatetemplate is still b0rken. The major issue is that $from will not necessarily be the same as the start of the preg_match; try passing separatetemplate("{{foox|xx}} {{foo|yy}} !!","foo"). There's also a second issue that you need a .* in "(\}.*)", or else 'end' is wrong when the template has no parameters.
It'll screw up if none of the $namesmigratefrom entries match; $list will be empty, and it might just blank the page.
templatesplit screws up if the template has no parameters, or if the last parameter is empty (e.g. {{foo|bar|baz|}}). In both cases, it claims an extra parameter containing "}".
templatesplit screws up if the template has an empty parameter, e.g. {{foo|bar||baz}}. When you reconstruct the template, suddenly {{{2}}} is baz and {{{3}}} is unset.

And a comment or two:

In the name migrator, if it's trying to migrate foo it won't match Foo.
In the field migrator, just do the preg_replace unconditionally. If there is no match, it'll work right.
It seems to work ok here, but there tends to be issues in PHP with foreach($x as &$v){ } unless you unset($v) after the end of the loop; in particular, if $v is assigned again before being unset it'll overwrite the last member of $x. It's up to you, but IMO better safe than sorry.

Anomie ⚔ 19:53, 7 March 2009 (UTC)[reply]

All these really are fixed now! The solution to your third point is a little hackish, but it will always work. With your fourth point, I beg to differ -- the name migrator was already case insensitive for the first letter. With your sixth point, I have indeed changed it to a for loop. Thanks! [[Sam Korn]] ^(smoddy) 20:26, 7 March 2009 (UTC)[reply]

I didn't notice earlier, but if you're going to stick a page name into a regular expressions, you have to make sure to properly escape any metacharacters:

$nameinsearch = "(?i:" . quotemeta(substr($namesmigratefrom[$i],0,1)) . ")" . quotemeta(substr($namesmigratefrom[$i],1));

Note also the use of the (?i:) group, which basically applies the 'i' flag to just that bit of the regex. Also, you applied the "found" check to the wrong namesmigratefrom loop: try this.

Other than that, looks good.

Approved for trial (30 edits). Please provide a link to the relevant contributions and/or diffs when the trial is complete. Anomie ⚔ 22:00, 7 March 2009 (UTC)[reply]

Trial complete.. The only issue was that the bot needlessly changed the capitalisation of template calls like this. Issue is now corrected. [[Sam Korn]] ^(smoddy) 13:25, 9 March 2009 (UTC)[reply]

Approved. Anomie ⚔ 14:27, 9 March 2009 (UTC)[reply]

The above discussion is preserved as an archive of the debate. Please do not modify it. Subsequent comments should be made in a new section.