mt-mods / technic

Technic mod for Minetest
18 stars 22 forks source link

Restore `technic.charge_tools` for compatibility #236

Open S-S-X opened 2 years ago

S-S-X commented 2 years ago

While looking for mods that might require updates for new power tools API I've noticed https://content.minetest.net/packages/gpcf/technictrain/ which required removed public function technic.charge_tools: https://git.bananach.space/technictrain.git/tree/init.lua#n44

I've removed (made local) said function myself here: https://github.com/mt-mods/technic/commit/968c64b4155df9b1d7c7762060ad7b94ee84b9e5

Should this be restored? Or ask external machines to utilize new technic.set_RE_charge(stack, charge) / technic.get_RE_charge(stack) functions?

First option is to provide compatibility (and increase API footprint which is bad for documentation and maintenance). Second option requires either another compatibility override (which might break things if original mod is updated) or updating mod code.

S-S-X commented 2 years ago

Personal opinion: function should not be restored because it is not really necessary for functionality and API filled with unnecessary methods is bad API.

OgelGames commented 2 years ago

Yeah, I think that would fall into the "never intended to be API" category, so I don't think it should be restored.

It definitely should be mentioned somewhere though, probably in the release changelog. (and a technic plus migration guide maybe?)

EDIT: Added to the release changelog here: https://github.com/mt-mods/technic/releases/tag/1.1.0

S-S-X commented 2 years ago

It definitely should be mentioned somewhere though, probably in the release changelog. (and a technic plus migration guide maybe?)

That function actually was not documented and looked pretty much like thing for internal use only, that technic train was only thing I could find when I started looking around for it. I don't think there's a lot of migration happening with that one but it is available on original technic mod. Could be mentioned somewhere but more important is getting actual current documentation updated and while doing that also look for redundant functions for cleanup (documenting as do not use this without any good explanation what it does).

OgelGames commented 2 years ago

Compared the technic namespace between minetest-mods/master and mt-mods/master, only other removed function is technic.smelt_item, which was not used at all, and was only removed recently in 03738e39a198a05e4d6e23169180d2648d016f74 (not in any releases yet).

OgelGames commented 2 years ago

also look for redundant functions for cleanup

This is probably useful then:

All the functions in technic namespace as of commit 713409b ``` activate_network add_network_branch add_network_node build_network can_insert_unique_stack chests.change_allowed chests.digiline_effector chests.get_formspec chests.get_inv_items chests.get_receive_fields chests.log_inv_change chests.move_inv chests.register_chest chests.send_digiline_message chests.sort_inv chests.update_formspec config.get config.get_bool config.get_flags config.get_int config.get_names config.get_np_group config.remove config.set config.set_bool config.set_np_group config.to_table config.write create_network default_can_insert disable_machine EU_string getter get_cable_tier get_max_lag get_or_load_node get_recipe get_RE_item_load get_timeout handle_machine_pipeworks handle_machine_upgrades insert_object_unique_stack is_overloaded is_tier_cable machine_after_dig_node machine_can_dig machine_inventory_move machine_inventory_put machine_inventory_take merge_networks network2pos network2sw_pos network_infotext network_node_on_dignode network_node_on_placenode network_run new_default_tube overload_network pos2network pretty_num radiation_callback refill_RE_charge register_alloy_furnace register_alloy_recipe register_base_machine register_battery_box register_cable register_can register_centrifuge register_compressor register_compressor_recipe register_electric_furnace register_extractor register_extractor_recipe register_freezer register_freezer_recipe register_generator register_grinder register_grinder_recipe register_machine register_power_tool register_recipe register_recipe_type register_separating_recipe register_solar_array register_tier remove_network remove_network_node reset_overloaded send_items set_default_timeout set_RE_item_load set_RE_wear swap_node sw_pos2network sw_pos2tier touch_node trace_node_ray trace_node_ray_fat tube_inject_item worldgen.gettext ```

And the WE command to get that:

//lua for a,b in pairs(technic)do if type(b)=="function"then print(a)elseif type(b)=="table"then for c,d in pairs(b)do if type(d)=="function"then print(a.."."..c)end end end end
S-S-X commented 2 years ago

I do like to use cli tools bit more as for example simple grep -nr 'function.*technic.*\|technic.*=.*function' gives pretty good results with file name, line number and code snippet. And add -A, -B or -C for context when needed.

BuckarooBanzay commented 2 years ago

My opinion: don't restore it

Worst case something like that could end up in some kind of "technic-shim" or "technic-compat" mod to make it backwards compatible (maintaining that would be a nightmare though)

S-S-X commented 2 years ago

Compatibility wrapper could be added for said train mod support but better of course would be to update mod itself, something like this with yelling about deprecated calls would probably work just fine:

function technic.charge_tools(meta, batt_charge, charge_step)
    local src_stack = meta:get_inventory():get_stack("src", 1)
    local def = src_stack:get_definition()
    if not def or not def.max_charge or src_stack:is_empty() then
        return batt_charge, false
    end
    local charge = technic.get_RE_charge(src_stack)
    charge_step = math.min(def.max_charge - charge, charge_step)
    technic.set_RE_charge(src_stack, charge + charge_step)
    meta:get_inventory():set_stack("src", 1, src_stack)
    return batt_charge, (def.max_charge == charge + charge_step)
end

Maybe I'll try to see if there's some simple way to make that happen in mod...

OgelGames commented 2 years ago

That's probably a good idea actually, because without that function that mod would crash, which is something we should be avoiding whenever possible. 👍

S-S-X commented 2 years ago

Now after #275 this can be done better with technic_get_charge/technic_set_charge:

function technic.charge_tools(meta, batt_charge, charge_step)
    local src_stack = meta:get_inventory():get_stack("src", 1)
    local def = src_stack:get_definition()
    if not def or not def.technic_max_charge or src_stack:is_empty() then
        return batt_charge, false
    end
    local new_charge = math.min(def.technic_max_charge, def.technic_get_charge(src_stack) + charge_step)
    def.technic_set_charge(src_stack, new_charge)
    meta:get_inventory():set_stack("src", 1, src_stack)
    return batt_charge, (def.technic_max_charge == new_charge)
end

Add to compatibility shims and include log warning about deprecated API function.

Warning because function is bad for API as it enforces inventory list name and inventory slots. Therefore it should not be included in official API but is good addition for compatibility until mods start using new API which is now also available and should be used with minetest-mods technic.