https://bugzilla.wikimedia.org/show_bug.cgi?id=69540
--- Comment #7 from Marius Hoch <[email protected]> --- Addressed the concerns raised by Brad (changes still in gerrit as of now). (In reply to Brad Jorsch from comment #2) > Reviewing revision 1441b9ab: > > Capiunto.php line 30: > > # XXX: Rather use ScribuntoExternalLibraryPaths ? > > ScribuntoExternalLibraryPaths is for pure-Lua libraries that are loaded > using require() from a module that wants them. > > ScribuntoExternalLibraries is for libraries that are loaded by default. A > library that is implemented with callbacks into PHP code currently needs to > be done using this hook, as the require() code path doesn't allow for > setting things up. > > Since this does appear to be a pure-Lua library that isn't of general > utility, ScribuntoExternalLibraryPaths probably would be a good way to go. > As we now call out to PHP in order to add the RL module, this is no longer be possible for mw.capiunto.Infobox (see below). But I've made mw.capiunto.Infobox._render a library which can be dynamically loaded, when needed. > Capiunto.hooks.php lines 54-55: > > $extraLibraries['mw.capiunto.Infobox'] = '\Capiunto\LuaLibrary'; > > $extraLibraries['mw.capiunto.Infobox._render'] = '\Capiunto\LuaLibrary'; > > This probably isn't right, it'll instantiate and call > \Capiunto\LuaLibrary::register() twice. Either the the classes should be > different for each, or just one entry should be made in $extraLibraries. > Fixed, now only mw.capiunto.Infobox is being added there. > Capiunto.hooks.php line 70: > > // @FIXME: Find a way to do this conditionally from Lua > > Ideally you'd probably want to do addModules on the ParserOutput during the > parse. If you were already using PHP callbacks that'd be easy, but providing > a way for a pure-Lua library to do it without also allowing modules to load > any random gadget could be tricky. > Implemented this with a custom method just for adding our Capiunto RL module (in order to get around having to expose something like that). > includes/lua/Infobox.lua line 28: > > error( name .. ' must be either of type string, number or nil' ) > > You'll most likely want to be specifying the 'level' parameter to your > error() calls,[1] so that the error is reported at the location of the > external caller rather than "Infobox.lua at line 28". The same goes for the > other error() calls in your code. > > [1]: http://www.lua.org/manual/5.1/manual.html#pdf-error > Done > includes/lua/Infobox.lua line 129: > > data = preprocess( data ), > > I see you're preprocessing a lot of the fields being passed in. Note that > when the fields are coming in from frame.args, they're already preprocessed > and shouldn't need to be preprocessed again; 'data' here seems particularly > likely to be in that situation. > I don't see a way around doing preprocess on everything right now... the problem is that we will get data from various sources and during some initial tests it turned out that preprocessing is something people just assume to be done. What we *could* do is add specific methods (or flags) that bypass (or enable?) preprocessing, but I'm not a huge fan of that. > includes/lua/Infobox.lua line 236: > > mw.capiunto = mw.capiunto or {} > > The evolving style would point to using mw.ext.Capiunto. > I'm not a fan of that as it seems overcomplicated. If you insist, I can change it, though. > includes/lua/InfoboxRender.lua line 28: > > :css( 'border-spacing', '3px' ) > > I have to wonder why this isn't in the stylesheet. Same for most of the > other :css() calls in here (lines 40, 67, 95, 107, 162, 189). Fixed. -- You are receiving this mail because: You are on the CC list for the bug.
_______________________________________________ Wikidata-bugs mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/wikidata-bugs
