Page MenuHomePhabricator

Get rid of globalvar in interwiki.py
Closed, ResolvedPublic

Description

I believe global variables are generally a bad practice. interwiki.py encapsulates them into class Global, but keeps using the single container global variable globalvar (which is an instance of Global). Not using global variables helps testing.

We could get rid of it by passing Global instances around as arguments. The affected classes would be Subject and InterwikiBot only.

A potential problem is breaking backward compatibility - if other scripts use class InterwikiBot, they would have accessed interwikibot.globalvar directly. [1] Could we accept that such scripts might break and they have to be updated?

If we are to do this, I also suggest renaming Global into InterwikiBotConfig or simply Config.

[1] See for example: https://gist.github.com/whym/4af2d44d586942ab03f51800b0398425/05decb25d78690fb6271458a96b1843677c9bb5b

Details

Related Gerrit Patches:

Event Timeline

whym created this task.Jun 17 2016, 12:27 PM
Restricted Application added subscribers: pywikibot-bugs-list, Zppix, Aklapper. · View Herald TranscriptJun 17 2016, 12:27 PM

Change 295107 had a related patch set uploaded (by Whym):
Get rid of globalvar in interwiki.py

https://gerrit.wikimedia.org/r/295107

whym added a subscriber: Xqt.EditedAug 11 2016, 10:11 AM

@Xqt said that:

I am a bit unsure about this patch because it is a breaking change and conf parameter to the affected classes is mandatory even it is a names parameter. Without the setting parameter the script wouldn't work. Otherwise it's only a script and it shouldn't derived anywhere.

Although I mentioned the possible incompatibility, I believe the number of the scripts dependent to interwik.py is really small. It is a script, not a library, after all.

As I mentioned, I'm introducing this in the belief that it would make writing tests easier. interwiki.py could use more extensive unit tests - there are so many corner cases with Wiktionary interwiki links. Crashes with Wiktionaries are so common that I have to restart it almost daily (with cron).

Change 295107 merged by jenkins-bot:
Get rid of globalvar in interwiki.py

https://gerrit.wikimedia.org/r/295107

Xqt closed this task as Resolved.Aug 26 2016, 5:12 AM
Xqt claimed this task.
Xqt reassigned this task from Xqt to whym.