Talk: Devmodule

Refactoring

Would you mind taking a look at Module:Devmodule/sandbox? I mostly just cleaned up the code, so it should work the same as it does now.

...Now that I think about it, I wonder how useful this module really is? I mean, let's say I want to use Module:Existsmod on my test wiki. Wouldn't it be just as easy to create a page called "Module:Existsmod" and paste:

return require('Dev:Existsmod')

Is there any benefit to using Module:Devmodule?

DarthKitty (talk) 02:58, October 3, 2015 (UTC)

Sure, it looks good. I'll merge it just now.

...Now that I think about it, I wonder how useful this module really is?

You'd have to do that for every single module you want to import. This provides a central point to get them without worrying about all that stuff. Especially if someone doesn't really want to create dozens of one line modules  simply for the sake of importing stuff from  dev.wikia.

Is there any benefit to using Module:Devmodule?

Yes, it reduces the need to create new modules for each import. It also allows one to test any module in dev.wikia without worrying about importing anything. 

You can see its usefulness in w:c:template wikia where I created only one module there, and several templates depend on it:

http://templates.wikia.com/wiki/Special:WhatLinksHere/Module:Devmodule

Dessamator (talk) 13:41, October 3, 2015 (UTC)

While templates do seem to require a local wrapper (at least, I tried to transwiki-load a module in a template #invoke statement, but got "This module doesn't exist" errors, but I may have been missing something), modules shouldn't at all - you should be able to have something like the following in a module, and have it work as expected:

local thing = require('dev:thing')

「ディノ奴千？！」^{? · ☎ Dinoguy1000} 23:19, October 3, 2015 (UTC)

I'm not sure I understand what you mean. Did you try to use something like {{#invoke:Links|main}} in another wiki that doesn't contain module:links or you tried to load it using this module?

If you tried the former, that will not work.

To use a direct "require", you need to bear in mind that it is case sensitive. So if you want to access module:thing from dev.wikia you need something like:

local thing = require("Dev:Thing")

Note the upper case "D" and "T". The confusing bit is that I added functionality to Module:devmodule that does this automatically.Dessamator (talk) 07:59, October 4, 2015 (UTC)

Close, I used {{#invoke:dev:thing|main}}. But with you mentioning that directly require'ing a module is case-sensitive (which I didn't know, since I still haven't done all that much with modules myself), it makes me wonder if case is what prevented it from working when I tried it, as well. 「ディノ奴千？！」^{? · ☎ Dinoguy1000} 08:51, October 4, 2015 (UTC)

...and it seems "Dev:Thing" also causes "No such module" errors, so that's not the case here. 「ディノ奴千？！」^{? · ☎ Dinoguy1000} 08:53, October 4, 2015 (UTC)

(reset indent)  The problem is this: {{#invoke:dev:thing|main}} It is not valid code, and the main reason I created this module. The only possible way to invoke (or use) a module stored in dev.wikia is through another module, e.g.:

{{#invoke:thing|main}}

 --Module:thing
   local thing = require("Dev:Thing")
   return thing

Unfortunately, a module with require is mandatory. Dessamator (talk) 13:36, October 4, 2015 (UTC)

V2

Could you take another look at the sandbox? I did so much refactoring this time that Devmodule is barely recognizable! Here are some highlights:

• Split cloneFrame() off of makePseudoFrame()
• Improved makePseudoFrame() such that it actually removes

1. mod et. al. (See how)

• Improved normalization for non-English wikis and moved it to its own function, normalize()
• Merged getInvokeDetails() and loadModule() into main()
• Improved error handling

I did some testing based on Module:Devmodule/testcases, and it seems like everything works. You should probably try it yourself, though! XD

DarthKitty (talk) 10:11, October 8, 2015 (UTC)

Nice, overall the updates seem good. But there are a couple of issues, see Module_talk:Devmodule/testcases#Sandbox tests:

• Error handling : It seems that you prevent the module from showing its errors.
  • Where the current devmodule fails silently, sandbox fails with a custom error
  • Where the current devmodule fails with an error from the called module, yours ignores the failure
  • Maybe you should show the traceback or the original error instead of unknown error, which doesn't help the user to understand what and where the error is.
  • Perhaps it would be better to remove that  callFrameMethod(clone, ...) from within the loop. It keeps being recreated on each loop.Dessamator (talk) 12:47, October 8, 2015 (UTC)

Could you write a separate module for testing purposes? It's a little... daunting, having to hunt through Module:Datecalc and then Module:Date for clues about my error handling code. On a related subject, why on Earth did you design Devmodule to fail silently??

DarthKitty (talk) 19:02, October 10, 2015 (UTC)

The test really applies to any module, as long as the wrong syntax is used. I just chose module:datecalc and module:date randomly. As far as failing silently, it wasn't really intended so you can add code to fail "loudly". I've created a module and added tests as requested.

Let's keep it simple:

• If the module name is missing or invalid return a simple error telling the user
•  If the function name is missing or invalid return a simple error telling the user
• If there's an error on the module being called let it show its error or its traceback, whichever is simpler, and more informative.

If you think any other errors are necessary feel free to add them too. Don't worry about making it identical to the current devmodule, the idea is to improve it.

Dessamator (talk) 22:17, October 10, 2015 (UTC)

Okay, I'm not sure what stupid mistake I made when I first wrote that error handling system, but I finally managed to beat it into submission. I also tweaked it so that errors from the #invoked module "fall through" Devmodule, and are passed to the user. Everything else is pretty much the same.

As far as errors go, the main differences from classic!Devmodule are:

• Improved messages
• Use of Module:User error instead of plain text and the error() function

...which is what I originally tried to do. These differences make three tests fail, but the values are actually correct.

DarthKitty (talk) 01:08, October 11, 2015 (UTC)

Nicely done. I've merged the sandbox code into the main module. Although, we probably need to add some testcases for localized (in other languages) module namespaces.

EDIT: I'm not so sure your error handling for function names will work well though. To answer your question in the code:

-- Why does Lua interpret `functionName` as a question mark, anyway?

It is not interpreting functionName as a question mark, it is indicating that a  variable field name is wrong.

For example:

local tableDolls = {}
tableDolls["ken"] = 1
tableDolls["getDolls"] = function () return 5 end

local functionName = "getD"
local variableName = "barbie"
tableDolls[functioName]  <--- attempt to call field '?'
tableDolls[variableName]  <--- attempt to call field '?'

In other words you need to return an error message if not (tableDolls[functionName]) then error("Function"..functionName.." doesn't exist").

Dessamator (talk) 10:50, October 11, 2015 (UTC)

I fixed a bug based on what I said above. Your implementation was blocking error messages from the invoked module.Dessamator (talk) 18:11, October 11, 2015 (UTC)

Better error handling (Part 3 of 1)

I rewrote the error handling, this time without parsing error messages. This greatly simplifies the main() function, and makes it much more difficult to block error messages. I will explain each of the seven failing tests, starting from the top:

1. I tweaked the error message
2. I tweaked the error message
3. I tweaked the error message
4. This is the original error from the #invoked module
5. I tweaked the error message
6. ???
7. This is the original error from the #invoked module

Four out of the seven failing tests are caused by tweaks to the error messages. Two occur because we're passing the original error to users, instead of a weird stringified variant. One failure might be the genuine article, but I have no clue what's causing it:

http://i.imgur.com/mM3pKvS.png

In case you're too lazy to click that link, the code {{#invoke:Devmodule/sandbox|main|}} allegedly returns a instead of a User error. However, I am unable to reproduce this behavior:

Please advise.

DarthKitty (talk) 21:56, October 11, 2015 (UTC)

That's an interesting bug. For some reason it seems to be outputting the error from the previous call:

{{#invoke:Devmodule/sandbox|main|#mod=Devmodule/testmodule}}

This might be a bug with Module:UnitTests or possibly a caching problem. We might have to move to Module:ScribuntoUnit, and see if it overcomes this error. As far as I can see there aren't any problems with your code, but more testing would be good.  Dessamator (talk) 08:31, October 12, 2015 (UTC)]

Merged

I've merged the latest sandbox changes. The bug was probably definitely due to something in Module:UnitTests. 

For future reference, when special tags are preprocessed their output  is not the same even if the same content is preprocessed in both cases (e.g. b.preprocess(<pre>abc</pre>) ~= a.preprocess(<pre>abc</pre>)). See the note in Wikipedia:Module:ScribuntoUnit#assertResultEquals.

Dessamator (talk) 12:58, October 12, 2015 (UTC)

Moar refactoring!!

I updated the sandbox again, and I'd appreciate a second pair of eyes before the code goes live. Here are the highlights:

• I cleaned up cloneFrame() a bit
• I almost completely rewrote normalize()
• I added #fun as an alias for #fname

On a related subject, Module:Devmodule/testcases doesn't run tests on the sandbox anymore. :(

DarthKitty (talk) 16:50, November 1, 2015 (UTC) Interesting updates, some comments:

• The clean up makes the code more readable. But you may want to make sure it returns nil for a grandparent, e.g. frame.getParent().getParent(). It might also be useful to remove this code from the function and create a meta-module. I have similar code in Module:Debug and Module:Utility. But a meta-module would be way better.
• isValidNamespace is a table, it probably shouldn't be named like that, because that "is" normally reserved for boolean. But the code is generally better and cleaner.
• I think #func makes more sense than "fun".

I just tested it out. It does run the sandbox tests, but you'd need to change the "testSandbox" table if you want to run different tests. test_sandbox_module() is the function running the sandbox tests.

On a side note, perhaps function main should probably just pass parameters to a function that will process it. That makes it easier to debug.

Dessamator (talk) 20:45, November 1, 2015 (UTC)

The clean up makes the code more readable. But you may want to make sure it returns nil for a grandparent, e.g. frame.getParent().getParent().

I just tried that on my test wiki and, apart from a minor bug in Module:Inspect, everything checks out. A cloned frame is identical to the original in every way. Incidentally, the code for frame:getParent() is more-or-less the same as what you wrote way back when:

• Before
• After

function self:getParent ()
    if (originalFrame:getParent()) then
        return pseudoFrame.new (originalFrame:getParent())
    end
end

function clone:getParent()
    local parent = frame:getParent()

    if parent then
        return cloneFrame(parent)
    end
end

It might also be useful to remove this code from the function and create a meta-module. I have similar code in Module:Debug and Module:Utility. But a meta-module would be way better.

I was thinking something similar, actually: this module is similar to Module:Existsmod, so it would make sense to combine them. Maybe we could do something like this, then?

1. Create a new module called "FrameTools" (or w/e).
2. Create a new module called "Modul" (or w/e), replacing Devmodule and Existsmod.
3. Move cloneFrame() and a slightly-abstracted makePseudoFrame() from Devmodule to FrameTools.
4. Move a slightly-abstracted normalize() from Devmodule to Modul.
5. Move main() from Devmodule to Modul as from_dev() (so you can {{#invoke:Modul|from_dev|...}}).
6. Move a slightly-refactored module_exists() from Existsmod to Modul as _exists().
7. Move main() from Existsmod to Modul as exists().
8. Deprecate Devmodule and Existsmod.

isValidNamespace is a table, it probably shouldn't be named like that, because that "is" normally reserved for boolean.

Okay, done.

I think #func makes more sense than "fun".

Me too, but "fun" is a more common abbreviation according to these sites: [1], [2], [3].

On a side note, perhaps function main should probably just pass parameters to a function that will process it. That makes it easier to debug.

I dunno... it seems like that would just make Devmodule more complicated:

• Before
• After

function p.main(frame)
    checkType("Dev:Devmodule.main", 1, frame, "table")

    local args = getArgs(frame)
    local moduleName = args["#mod"] or args["#modulename"]
    local functionName = args["#fun"] or args["#fname"] or "main"
    local success, invokedModule

    -- try to `normalize()` the given module name
    -- if this fails, then the user did not pass the right parameters
    success, moduleName = pcall(normalize, moduleName)

    if not success then
        return userError("no `#mod` parameter given " ..
                         "(should be the name of a module on dev.wikia)")
    end

    -- try to `require()` the given module
    -- if this fails, then the user gave an incorrect module name
    success, invokedModule = pcall(require, "Dev:" .. moduleName)

    if not success then
        return userError("could not find a module called `" .. moduleName ..
                         "` on dev.wikia")
    end

    -- make sure the module has a function called `functionName`
    -- if this fails, then the user gave an incorrect function name
    local func = invokedModule[functionName]

    if not func then
        return userError("that module does not have a function called `" ..
                         functionName .. "`")
    end

    -- call the function
    -- any errors from the function itself will be passed to the user
    local pseudoFrame = makePseudoFrame(frame)

    return func(pseudoFrame)
end

local function p._main(moduleName, functionName)
    local success, invokedModule

    -- try to `normalize()` the given module name
    -- if this fails, then the user did not pass the right parameters
    success, moduleName = pcall(normalize, moduleName)

    if not success then
        return userError("no `#mod` parameter given " ..
                         "(should be the name of a module on dev.wikia)")
    end

    -- try to `require()` the given module
    -- if this fails, then the user gave an incorrect module name
    success, invokedModule = pcall(require, "Dev:" .. moduleName)

    if not success then
        return userError("could not find a module called `" .. moduleName ..
                         "` on dev.wikia")
    end

    -- make sure the module has a function called `functionName`
    -- if this fails, then the user gave an incorrect function name
    local func = invokedModule[functionName]

    if not func then
        return userError("that module does not have a function called `" ..
                         functionName .. "`")
    end

    -- call the function
    -- any errors from the function itself will be passed to the user
    local pseudoFrame = makePseudoFrame(frame)

    return func(pseudoFrame)
end

function p.main(frame)
    checkType("Dev:Devmodule.main", 1, frame, "table")

    local args = getArgs(frame)
    local moduleName = args["#mod"] or args["#modulename"]
    local functionName = args["#fun"] or args["#fname"] or "main"

    return p._main(moduleName, functionName)
end

DarthKitty (talk) 23:16, November 1, 2015 (UTC)

I just tried that on my test wiki and, apart from a minor bug in Module:Inspect, everything checks out.

Nice to know. When I was writing Module:Debug I had some problem with infinite recursive loops. getParent kept calling itself.

I think devmodule and existmod fulfill different roles. Currently, we can mess around with existmod without worrying about breaking devmodule and vice versa. Existmod is also a light module that can be loaded without all the baggage of devmodule.

I do however like the idea of https://dev.fandom.com/wiki/Module:FrameTools.

I dunno... it seems like that would just make Devmodule more complicated:

That's true as long as function main doesn't become much longer than it currently is. For now it might be better to leave it as is because we can't debug it anyway without using the frame which is not accessible using the console.

Dessamator (talk) 23:26, November 2, 2015 (UTC)

Okay, the sandbox now uses Module:FrameTools! I also updated Module:Existsmod/sandbox with the same normalize() function we're using here (more-or-less). Since those two are so similar, it makes sense to combine 'em—the only question is where.

• On one hand, we could create another meta-module with just that normalization code. The problem is, it isn't connected to anything else (besides Devmodule and Existsmod, that is).
• On the other hand, we could merge Devmodule and Existsmod, and have them share that normalization code. Unfortunately, that new module would be more complicated (and maybe slower).

I still think merging those two modules is the lesser evil, but I'm okay with extracting normalize() if you want.

DarthKitty (talk) 05:54, November 3, 2015 (UTC)

On one hand, we could create another meta-module with just that normalization code. The problem is, it isn't connected to anything else (besides Devmodule and Existsmod, that is).

It probably makes more sense to make another meta-module. Despite its obvious "evilness" one could argue that data-modules are worse because they are often used only used by a single module, at least normalize would be used by two. Dessamator (talk) 08:42, November 3, 2015 (UTC)

All right, I extracted normalize() into its own module: Module:NormalizeModuleName. As you have probably gathered by now, I'm not a fan of this solution, but maybe we can find a better home for that code in the future... The good news is, this change makes both Devmodule and Existsmod smaller than they were before I began to refactor them!

DarthKitty (talk) 21:33, November 3, 2015 (UTC)

Yes, it makes it far easier to debug too. I've merged the changes. Dessamator (talk) 07:02, November 4, 2015 (UTC)