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.
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
toneeds-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
[edit]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.
- BTW, isn't a regex of
- 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 passingseparatetemplate("{{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 matchFoo
. - 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 youunset($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.
- In the name migrator, if it's trying to migrate
- 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]
- 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:
- 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
- There's a new bug or two:
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.