-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add insertAfterTemplates
to Morebits.wikitext.page, respect MOS:ORDER when tagging, update hatnotes
#1022
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty neat. It would be great if you can include some unit tests too, as you go about testing this. Would make any future changes much easier to verify.
twinkle.js
Outdated
@@ -126,6 +126,9 @@ Twinkle.defaultConfig = { | |||
// Hidden preferences | |||
autolevelStaleDays: 3, // Huggle is 3, CBNG is 2 | |||
revertMaxRevisions: 50, // intentionally limited | |||
// various hatnote templates, used when tagging | |||
// (csd/xfd/tag/prod/protect) to ensure MOS:ORDER | |||
hatnoteRegex: 'short description|hatnote|main|correct title|dablink|distinguish|for|further|selfref|year dab|similar names|highway detail hatnote|broader|about(?:-distinguish| other people)?|other\\s?(?:hurricane(?: use)?s|people|persons|places|ships|uses(?: of)?)|redirect(?:-(?:distinguish|synonym|multi))?|see\\s?(?:wiktionary|also(?: if exists)?)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a user preference at all. What we're missing is a way to share data/code between modules. It would be better to define this as Twinkle.hatnoteRegex
in twinkle.js
file at the bottom under a header called something like:
/**
* Data and code shared between multiple modules,
* but not generic enough to be in morebits.
*/
Such a section will also come handy for a lot of other stuff too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly right. I wrote it that way at first, but shied away from it because there simply wasn't anything else there. I thought having it in defaultConfig
might make it more obvious that "this is a thing to change," but, yes, it'd be better to just do it normally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a good place for #969
morebits.js
Outdated
|
||
// Regex is extra complicated to allow for templates with | ||
// parameters and to handle whitespace properly | ||
return this.text.replace( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this.text.replace( | |
return this.text = this.text.replace( |
At the least, this should modify this.text
. I'm not sure whether it'd better to return the modified text or just this
(which would make the method chainable). Presently, the other Morebits.wikitext methods don't return anything so changing them all to return this
is what would make sense IMO, so that it can be used like text = wikipage.insertAfterTemplates(...).getText()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that makes sense, and chaining would be ideal. I can toss in some return this
-es to the others, IIRC most of the uses are simple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused on enwiki, so added here.
morebits.js
Outdated
// .length is only a property of strings and arrays so we | ||
// shouldn't need to check type | ||
if (!regex || !regex.length) { | ||
return tag + this.text; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to throw when the input is clearly invalid, so that the client can know they're doing something wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair, although I'm mixed on how much of an error/invalid we should actually consider these. I guess it can just be handled on the "client" side, but this seemed nicer. You're probably right about giving them the option of wanting to know.
Would you think both lack of tag and lack of
regex` should throw, or just one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went with both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose technically we could check the flags, something like /^[gimsuy]$/
Toss `return this` at the end of the methods. Uses of `removeLink` and `commentOutImage` were pretty simple and amenable to chaining; `removeTemplate` and `addToImageComment` are currently unused by Twinkle.
Takes the structure of the regex from the `tag` module, and uses it in `Morebits.wikitext.page.prototype.insertAfterTemplates` to properly find complex templates in the page wikitext, then insert a given string after them. The regex is identical to what has been used in `tag`: - The regex for a series of templates is provided as either a string or as an array, from which each item will be pipe-separate. This is what in `tag` is the CSD, PROD, and "various hatnote templates" section. - An optional regex flag can be provided; default is `\i`. - There's an optional `preRegex` for stuff that comes *before* the templates proper (i.e., before `{{`). This is mainly because enWiki's AfD has an annoying hidden html comment before the `/dated` transclusion. We could add `postRegex` in the future, although that's likely overengineering
Uses `Morebits.wikitext.page.insertAfterTemplates` with a customized regex string in the hidden preference string `hatnoteRegex`. `tag` adds on the deletion templates (CSD/PROD/XfD), and finally learns about protection templates.
Barely been updated in a decade; `the` was deleted (https://en.wikipedia.org/wiki/Wikipedia:Templates_for_discussion/Log/2011_January_17#Template:The) in 2011!!! Includes most templates from https://en.wikipedia.org/w/index.php?title=Template:Hatnote_templates with more than ~100, 150 transclusions. A few commonish redirects included as well.
Was part of wikimedia-gadgets#1022 for `insertAfterTemplates`, but wasn't present for `removeLink`, `commentOutImage`, `addToImageComment`, and `removeTemplate`.
Was part of wikimedia-gadgets#1022 for `insertAfterTemplates`, but wasn't present for `removeLink`, `commentOutImage`, `addToImageComment`, and `removeTemplate`.
As noted on WT:TW, Twinkle isn't great about MOS:ORDER when tagging things. Accordingly, this:
tag
. Takes either a string or an array and allows for custom flags.{{pp}}
,{{pp-move}}
, etc.). A new hidden preference,hatnoteRegex
, is added to store the "various hatnotes templates` regex in a unified location.There are some more notes in the individual commits. The main annoyance is that AfD has a comment before the actual template, so having to support that as well is a drag. Maybe the option to specify a flag for the regex for
insertAfterTemplates
is unnecessary?