Legacy compat

[BuckarooBanzay]

I found this in the wild:

if minetest.get_modpath("wrench") and wrench then
    local STRING = wrench.META_TYPE_STRING
    wrench:register_node("bitchange:shop", {
        lists = {"stock", "custm", "custm_ej", "cust_ow", "cust_og", "cust_ej"},
        metas = {
            owner = STRING,
            infotext = STRING,
            title = STRING,
        },
        owned = true
    })
end

Source: https://github.com/SmallJoker/bitchange/blob/733ce6f46a0fe3cd28bc4e2429c3b6497201f50c/shop.lua#L467-L478

Correct me if i'm wrong, but i'm pretty sure this wont work with the current rewrite :/ how do we handle cases like this?

[S-S-X]

I think API was fully compatible after rewrite done by @OgelGames? Would it be possible to keep that compatibility with latest update adding more nodes (if there's something that would make it incompatible)?

[S-S-X]

Actually yes, old API is not supported because function signature was changed from: register_node(self, name, definition) to register_node(name, definition) ...

I think it would be best to correct that by restoring old function signature, there's no real reason to not use that. Well, another dirty workaround would be to check first argument and skip it if it is not string type.

Previous was:

function wrench:register_node(name, def)
    assert(type(name) == "string", "wrench:register_node invalid type for name")
    assert(type(def) == "table", "wrench:register_node invalid type for def")
    if minetest.registered_nodes[name] then
        self.registered_nodes[name] = def
    else
        minetest.log("warning", "Attempt to register unknown node for wrench: "..tostring(name))
    end
end

And after last rewrite it became:

function wrench.register_node(name, def)
    assert(type(name) == "string", "wrench.register_node invalid type for name")
    assert(type(def) == "table", "wrench.register_node invalid type for def")
    local node_def = minetest.registered_nodes[name]
    if node_def then
        ... some magic here
    else
        minetest.log("warning", "Attempt to register unknown node for wrench: "..name)
    end
end

[OgelGames]

digilines solved the problem with this: https://github.com/minetest-mods/digilines/blob/master/init.lua#L4-L25

Maybe we could add the same thing in legacy.lua? Probably not worth it for a single function though...

Probably should also add wrench:original_name(name) in there too, just to prevent crashes with anything that used it.

[S-S-X]

digilines solved the problem with this: https://github.com/minetest-mods/digilines/blob/master/init.lua#L4-L25

Maybe we could add the same thing in legacy.lua? Probably not worth it for a single function though...

Probably should also add wrench:original_name(name) in there too, just to prevent crashes with anything that used it.

That's terrible when you can just:

function wrench.register_node(a, b, c)
  local name = type(a) == "string" and a or b
  local definition = type(a) == "string" and b or c

[OgelGames]

This should suffice:

local register_node = wrench.register_node

function wrench.register_node(self, ...)
    if self == wrench then
        register_node(...)
    else
        register_node(self, ...)
    end
end

function wrench.original_name(self, name)
    if self ~= wrench or type(name) ~= "string" then
        return
    end
    if name:find("wrench:picked_up_") then
        for n in pairs(wrench.registered_nodes) do
            if name:find(n:gsub(":", "_")) then
                return n
            end
        end
    else
        return name
    end
end

[nixnoxus]

is there anything against writing it back from wrench.register_node to wrench:register_node? wouldn't that be cleaner?

[S-S-X]

is there anything against writing it back from wrench.register_node to wrench:register_node? wouldn't that be cleaner?

IMO would work fine as it is very unlikely that anyone already used new API. Also good thing would be that it is cleanest solution and there's absolutely no practical downsides, assertions also make it very clear that call is done wrong and examples in existing mods would be correct.

[nixnoxus]

I tried that:

create a dummy mod:

# cat mods/minetest_hacks/mod.conf
name = minetest_hacks
optional_depends = "wrench"

# cat mods/minetest_hacks/init.lua 
local modpath = minetest.get_modpath("minetest_hacks")

print("###### load minetest_hacks")

if minetest.get_modpath("wrench") and wrench then
    local STRING = wrench.META_TYPE_STRING
    print("###### " .. STRING .. " ##")
    wrench:register_node("default:dirt", {
        lists = {"stock", "custm", "custm_ej", "cust_ow", "cust_og", "cust_ej"},
        metas = {
            owner = STRING,
            infotext = STRING,
            title = STRING,
        },
        owned = true
    })
end

adjust wrench mod:

# sed -i 's@wrench.register_node@wrench:register_node@g' mods/wrench/*.lua mods/wrench/nodes/*.lua

the wrench works, but the dummy mod reports:

###### load minetest_hacks
2021-12-04 18:18:47: WARNING[Main]: Undeclared global variable "wrench" accessed at ...est/minetest.git/bin/../mods/minetest_hacks/init.lua:5

the error message also comes with

• https://github.com/mt-mods/technic 5aeeb27
• https://github.com/minetest-mods/technic master

Did I miss something? Or is the 'wild code' (from the very top) generally incorrect?

update: okay, that's settled. with https://github.com/SmallJoker/bitchange it works. was my mistake

[OgelGames]

I think we should NOT change back to :, it's just not correct, : is for OOP, which wrench should not be.

I don't think we even need to provide compatibility, anything using it was using something that was broken, and should be updated.

[nixnoxus]

new suggestion:

• we stay at . in extended_wrench_support but name the mod "wrench2".
• for compatibility stay with : in modpack technic
• after the release of extended_wrench_support as "wrench2", mod wrench in modpack technic serves as wrapper to mod wrench2

advantage: compatibility & clean code

[OgelGames]

No, that's just confusing, and wrapping it from technic is a bad idea.

There are two options:

1. Change nothing, and force the (already broken) mods to update.
2. Add a simple compatibility wrapper in legacy.lua to check the first argument.

[S-S-X]

1. Change nothing, and force the (already broken) mods to update.

How mods were broken exactly? Or how mods are broken with current released wrench mod?

2. Add a simple compatibility wrapper in legacy.lua to check the first argument.

IMO to change syntax which is not OOP in any way by itself legacy.lua is just over complicated way to solve simple problem. Also : is not OOP by itself, it is simply syntactic sugar to provide access to self. It can be used for OOP Lua but it does not make Lua object oriented in any way.

Either add compatibility which can be clean and simple or keep old non OOP : syntax, both can provide seamless upgrade from both other Technic forks and original Technic while keeping compatibility with those.

[nixnoxus]

the legacy.lua option would also be okay for me.

[OgelGames]

How mods were broken exactly? Or how mods are broken with current released wrench mod?

The API was broken before the rewrite, picking up a node registered from another mod resulted in an unknown item.

[S-S-X]

In that case forget about legacy and "compatibility" because there's no compatibility to add. Unless there's other Technic forks that had wrench fixed already and through that made mods compatible like one providing bitchange:shop. Any evidence of such Technic forks? At least not on ContentDB.

Maybe just add PRs for known mods that used broken wrench API which was not usable or useful at all and explain that it never worked so there's no loss in making it "incompatible" with original Technic wrench and only gain by making it compatible with new wrench.

I mean at this point adding compatibility is just adding compatibility for "legacy" stuff that never existed, yes there's some code somewhere but it never did anything useful.

Main question is: Was there ANY mods that were actually compatible with old wrench registration API? As I understood it none of mods were compatible with wrench, there was simply some code in other mods that never worked in any way.

[OgelGames]

Maybe just add PRs for known mods that used broken wrench API which was not usable or useful at all and explain that it never worked so there's no loss in making it "incompatible" with original Technic wrench and only gain by making it compatible with new wrench.

That's probably the best solution 👍 Probably need to add a way to identify the new wrench though, otherwise the mods will crash with the original version.

[OgelGames]

I added wrench.plus = true, the same as what was decided for technic.

So for any mods that want to support wrench, they should do this:

if minetest.get_modpath("wrench") and wrench.plus then
    -- wrench.register_node(name, def)
end

[nixnoxus]

I tested https://github.com/SmallJoker/bitchange/ with @OgelGames (reverted) commit http://github.com/mt-mods/wrench/commit/302173124ade4d433c1a7f443c158e03e14e0e7f.diff and it works.

Under these circumstances, i would be in favor of accepting this commit after all. It would be more user friendly.

[S-S-X]

Under these circumstances, i would be in favor of accepting this commit after all. It would be more user friendly.

Would be also simpler for updates and is near cleanest solution that wont require changing other mods. It seems like everyone agrees there.

[nixnoxus]

Done https://github.com/mt-mods/wrench/pull/2/commits/76cbf7ede7efdb7b4e3c8521e03df965a042df7c