Module talk:Flags

Code change

Hello I saw your module on it.wiki, and I would like to make some changes to the code:

declare local all variables, they are more efficient and quick to access and you dont pollute global namespace
Why do you use a for loop to check if a key is in the table like:

for flagParameter,commonsFile in pairs(master.twoLetter) do 
    if flagParameter == territory.args[1]  then 
       commonsName = commonsFile 
       tempLink = commonsFile
     end
end

is simpler and quicker try to assign and check for nil

commonsFile = master.twoLetter[territory.args[1]]
if commonsFile then --check for nil
    commonsName = commonsFile 
    tempLink = commonsFile
end

3. Why check for if territory.args[2] ~= "{{{2}}}" then ? AFAIK a paremeter call will be resolved before call the module and substituded by its value.

I will upload my changes to the sandbox for test.--Moroboshi (talk) 11:14, 22 July 2013 (UTC)[reply]

Thank you for the interest and the good advice! This is not only my first attempt with Lua programming, it is also my most serious attempt to program anything out of HTML/CSS. For instance, I wasn't aware that by not declaring variables explicitly local the module would pullute the global namespace (something that sounds obvious now that you say it).

Your approach in 2) is definitely smarter and I'm happy to take it after testing that there are no side effects (I don't see why there should be any).

if territory.args[2] ~= "{{{2}}}" then is the way I found to check whether a parameter for variants i.e. "1945" has been defined, after asking at mediawiki.org. Improvements welcome.

Now I comprend you check for a parameter not definited. I think its cleaner to set the parameter to {{{2|}}} in the template and check for null string "" in the module.--Moroboshi (talk) 18:01, 22 July 2013 (UTC)[reply]

The same for {{{link}}} or {{{text}}}, set them to {{{link|}}} or {{{text|}}} and check for null string in the module. I uploaded the modified flags to Module:Flags/Sandbox with some comments about my changes. Apparently it works with the example test in the doc.--Moroboshi (talk) 18:49, 22 July 2013 (UTC)[reply]

I had a first look at you version in the sandbox. Looks good, and you are saving almost 20 LOC! But I want to be cautious taking the changes since the template is already transcluded in 600 articles at ca.wiki. Where is git when you need it? ;) I'm starting my holidays tomorrow, flying and then having less time in front of the laptop. Still, I should be able to connect in the evenings. Feel free to "merge" your changes, just trying to do it edit by edit with comments for posterity. Welcome to the team! :)

Just curious: why are you changing the sequence of the script? It makes comparing code a bit more difficult. I had the idea of going for 1 Name; 2 Variant; 3 Size; 4 Link; 5 Label - but if there are good reasons to change it I'm all ears.

I had trouble with other approaches for {{{link}}} or {{{text}}}. Null string is actually different from no parameter defined. Users can set link= or text= to give specific instructions to the template (no link and same label as name of flag).

I think the only thing I moved out of order was the function default_size (and obviously check_translation that is new), i assigned the name flag_code to territory.args[1] because is used in different points of the script (but maybe the name could be more specific).
Regarding the use of parameters do you known that you can simplify the template call removing all parameters like {{Invoke|Flags|flag}} and access the parameters used in the template call directly in the lua module using territory:parent().args() ? Is sufficient to add this code to the start of the function:

function p.flag(territory)
    -- get the frame of the calling calling template
    local pframe = territory:getParent()
    -- get all parameter and store them in a table (pframe return a table with limited function, see scribunto documentation)
    local args = {}
    for k, v in pairs( pframe.args ) do
        args[k] = v
    end

And then you can check the parameter in the standard way, args[1] por positional parameter 1 and args['text'] or args.text for named parameter "text". If text is not definet it will be nil, if its defined but empty text= then it will be equal to the null string "":

Good holiday, I will fly next week. --Moroboshi (talk) 22:25, 23 July 2013 (UTC)[reply]