Module talk:Color contrast

Making the lum function accessible from modules

I've made the lum function accessible from modules by changing the code from:

function p.lum(frame)
	return color2lum(frame.args[1] or frame:getParent().args[1])
end

to:

function p.lum(frame)
	local args = frame.args[1] or frame:getParent().args[1]
	return p._lum(args)
end

function p._lum(args)
	return color2lum(args)
end

The changes are on the sandbox and will implement them if there are no comments. --Gonnym (talk) 11:03, 6 January 2019 (UTC)

@Gonnym: That looks like a very sensible and useful improvement. My only reservation is that passing the parameters as a table gives the person wanting to import the routine no idea of what values/types to supply to the routine without having to read through all of the code to determine them. In general I'd usually recommend either documenting a brief list of required values for the args table or passing the parameters as a list of obviously-named variables rather than a table. But that's just a minor point and shouldn't stop you from updating the main module. --RexxS (talk) 13:03, 6 January 2019 (UTC)

@RexxS: I don't mind doing either if that helps. I just followed the current style used for the other three public functions. Do you want me to change _lum(args) to _lum(color)? --Gonnym (talk) 13:27, 6 January 2019 (UTC)

@Gonnym: Sorry, my confusion: now I've looked harder at it, you're actually passing a string representing the colour, not a table (which is what args would most commonly indicate). I would suggest:

-- use {{#invoke:Color_contrast|somecolor}} directly or {{#invoke:Color_contrast}} from a wrapper template:
function p.lum(frame)
	local color = frame.args[1] or frame:getParent().args[1]
	return p._lum(color)
end

-- This exports the function for use in other modules.
-- The colour is passed as a string:
function p._lum(color)
	return color2lum(color)
end

I know we can put fuller information in the documentation, but I always suggest leaving small annotations in the code to help any re-users.

You could also write p._lum = color2lum instead of the second function definition, but setting it out explicitly, as you have done, helps whoever is importing the module to see what parameters to supply. Cheers --RexxS (talk) 15:41, 6 January 2019 (UTC)

Sure, looks good. I'll update the sandbox version. --Gonnym (talk) 15:43, 6 January 2019 (UTC)

Possible typos in code?

Observing the code, I might have spotted a couple of typos:

In p._greatercontrast function, lines 160 and 161

I think

if mw.ustring.match(v3, '^[A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9]$') or
	mw.ustring.match(v3, '^[A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9]$') then

should read

if mw.ustring.match(c3, '^[A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9]$') or
	mw.ustring.match(c3, '^[A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9][A-Fa-f0-9]$') then

instead. Here, v3 is expected to be a number, thus there is no point in applying it a regular expression. Besides, given the four lines above, it seems logical to use c3 variable here instead of v3.

In p._styleratio function, line 209

I think

if( lum ~= '' ) then bg, lum_fg = v, lum end

should read

if( lum ~= '' ) then fg, lum_fg = v, lum end

instead, since at this point we have isolated a forecolor specification from input CSS.

My apologies if I would have misinterpreted your code.

Kindly, Olivier Ph. (talk) 12:11, 13 October 2020 (UTC)

Categorisation broken?

Through Documentaion subpage, Category:Modules handling colors is added as expected. However, the page does not appear in Category:Modules handling colors (3). -DePiep (talk) 15:12, 7 January 2021 (UTC)