factoriolib / flib

A set of high-quality, commonly-used utilities for creating Factorio mods.
https://mods.factorio.com/mod/flib
MIT License
61 stars 15 forks source link

Allow some calback functions to take addtl args #40

Closed Nexela closed 2 years ago

Nexela commented 2 years ago

Fix up EmmyLua Add flib_table.unique_insert()

Nexela commented 2 years ago

Adding varargs to call back functions allows one to extract out the callback to a reusable function

local callback(v, k, mydata)
  if v.whatever == mydata.whatever then return true end
end

script.on_event(blah, function()
  table.for_each(sometable, callback, someothertable)
end)
JanSharp commented 2 years ago

Regarding varags for callbacks I'd just like to note that the vast majority of the time they are not needed because the callback functions are likely defined "in place" as a function expression allowing them to capture locals in the parent functions as upvalues. While varags don't hurt here I'd suggest thinking about wether or not you want to support the most common case or also support the rare special cases. I personally have grown to like writing libraries with the most common case in mind, but still allowing for the more rare cases at the cost of more complexity for those specific cases, keeping the complexity for those cases out of the library itself, making it easier to learn.

For example compare the example Nexela provided (this one):

local callback(v, k, mydata)
  if v.whatever == mydata.whatever then return true end
end

script.on_event(blah, function()
  table.for_each(sometable, callback, someothertable)
end)

With this:

local callback(v, k, mydata)
  if v.whatever == mydata.whatever then return true end
end

script.on_event(blah, function()
  table.for_each(sometable, function(v, k)
    callback(v, k, someothertable)
  end)
end)

The latter is obviously bigger and slightly less performant (although if you're using for_each you're already sacraficing some performance), however it removes complexity from the library and increases verbosity in the written code which - imo - makes it easier to read in the future. Making code easier to read reduces the chance of breaking code when refactoring or simply getting confused.

Something to consider, it is a minor thing for sure but I felt like it was worth noting even just as something to keep in mind for future features.

raiguard commented 2 years ago

I agree with JanSharp. I generally am not a fan of varargs in general - they can cause all sorts of strange behavior. I don't know why I said I liked them when I reviewed, because that's actually not the case. I guess I was tired or something.

raiguard commented 2 years ago

JanSharp and I both aren't fans of the varargs, so I'm going to close this PR. If you want to add table.unique_insert(), please open a new PR for it.