-
Notifications
You must be signed in to change notification settings - Fork 23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Restore technic.charge_tools
for compatibility
#236
Comments
Personal opinion: function should not be restored because it is not really necessary for functionality and API filled with unnecessary methods is bad API. |
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 |
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. |
Compared the |
This is probably useful then: All the functions in technic namespace as of commit 713409b
And the WE command to get that:
|
I do like to use cli tools bit more as for example simple |
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) |
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... |
That's probably a good idea actually, because without that function that mod would crash, which is something we should be avoiding whenever possible. 👍 |
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. |
technic.charge_tools
for compatibility
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#n44I've removed (made local) said function myself here: 968c64b
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.
The text was updated successfully, but these errors were encountered: