Module talk:Flags

Code change[edit]

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]

Changes tested and taken. Thank you very much!--QuimGil (talk) 03:56, 22 August 2013 (UTC)[reply]

Problem with File:Flag of the Soviet Union 1955.svg[edit]

There seems to be a problem with the flag for the Soviet Union. This is affecting at least 49 pages (the Winter Olympics articles at Category:Articles with missing files). One file being called is File:Flag of the Soviet Union 1955.svg, which seems to be a redirect to File:Flag of the Soviet Union (1938-1980).svg. Other Soviet Union flags redirecting to that one also seem to be missing, causing the bigger problem. - tucoxn\^talk 01:11, 13 February 2014 (UTC)[reply]

Please spell it out with an example for dummies unfamiliar with the topic. I would like a link to an article and a brief description of how I can see there is something wrong (I don't mean the category, I mean where is the error in the article). Johnuniq (talk) 07:07, 13 February 2014 (UTC)[reply]

I'd better add User:Tucoxn for a ping. Johnuniq (talk) 07:09, 13 February 2014 (UTC)[reply]

Johnuniq: Thanks for your response but it looks like this got fixed. It looks like this was an issue with some changes on Commons due to a rename request. Here are some Commons file histories that might explain what the problem was: history for "File:Flag of the Soviet Union (1938-1980).svg", history for "File:Flag of the Soviet Union 1955.svg", and history for "File:Flag of the Soviet Union (1955-1980).svg" (a new file). Again, this seems fixed for now. Thanks! - tucoxn\^talk 09:43, 13 February 2014 (UTC)[reply]

Performance[edit]

Is this module faster or slower than the old flag templates based on Country data system? Petr Matas 01:15, 22 June 2014 (UTC)[reply]

I have done some measurements myself. Analysis of a page with 50 {{Flagicon}}s takes 429±65 ms, analysis of a page with 50 {{Flags}} takes 472±66 ms of CPU time. The performance difference is lower than the measurement error. Petr Matas 10:47, 23 June 2014 (UTC)[reply]

Perhaps it would help performance to convert Module:Flags/MasterData to a JSON page and load its contents using mw.text.jsonDecode(). – Minh Nguyễn ^💬 05:33, 13 June 2019 (UTC)[reply]

Chuuk creates a link to a dab page[edit]

{{flags|Chuuk}} creates a link to a dab page Chuuk. It should like to the article Chuuk State instead. Here's an illustration: . Can someone fix that? Coastside (talk) 05:16, 20 January 2020 (UTC)[reply]