Skip to content
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

Replace mesecon.mergetable #533

Merged
merged 4 commits into from
Dec 19, 2020
Merged

Replace mesecon.mergetable #533

merged 4 commits into from
Dec 19, 2020

Conversation

numberZero
Copy link
Contributor

@numberZero numberZero commented Sep 10, 2020

Fix #483.
The two mergetable use cases were in fact unrelated, so now there are two functions. The original mergetable is now deprecated. So, are there any standard ways to track and report its uses?

@SmallJoker
Copy link
Member

Was this function documented somewhere? If it's an internal function, then replacing it would be fine. Otherwise output a deprecation warning so that modders fix it within the next two years (or so).

I could grep through my mod collection to figure out whether it was used in any other mod. However, if it's documented it is definitely in some obscure mod, causing breakages.

@numberZero
Copy link
Contributor Author

@SmallJoker The function wasn’t documented but undocumented functions are used from time to time (like wallmounted_get, see #520 for details); lack of documentation doesn’t imply the function is internal as little is documented in Mesecons (see also #530).

output a deprecation warning

Is there a standard way to do that that won’t flood the log even if the function is used in a hot path?

@SmallJoker
Copy link
Member

Uses of mesecon.mergetable in 349 different mods (mostly popular ones):

mesecons/mesecons_extrawires/mesewire.lua
mesecons/mesecons/util.lua
mesecons/mesecons/internal.lua
mesecons/mesecons/presets.lua

No external uses.

Is there a standard way to do that that won’t flood the log even if the function is used in a hot path?

So far I think it's only possible to log it once, or make use of debug functions to log it once per caller function. Simplest:

local was_warned = false
...
	if not was_warned then
		minetest.log("deprecated", "Deprecated call to mesecon.mergetable .......")
		was_warned = true
	end

Ironically, this "log level" isn't documented either...

@numberZero
Copy link
Contributor Author

I think plain logging should be OK. The function is to be removed so the warning should be prominent, and should point to the caller. Risk of flooding is minimal as there are no known uses at all.

P.S. grep would be most useful over ContentDB, is that possible?

@rubenwardy
Copy link
Contributor

List of packages on ContentDB that have mesecon.mergetable in their .zips:

         title          
------------------------
 Dreambuilder
 Mesecon Node
 Mesecons
 MineClone 2
 Minetest Essentials
 Minetest Extended
 Minetest Game Improved
 Regnum
 Regnum 2
 Voxel Dungeon
 Whynot
(11 rows)

@rubenwardy
Copy link
Contributor

Dreambuilder and Mesecon Node are mod/modpacks

@numberZero
Copy link
Contributor Author

Each entry in the list except of Mesecon Node, be it a game or a modpack, contains a copy of Mesecons.

@numberZero
Copy link
Contributor Author

@numberZero
Copy link
Contributor Author

^ Notified the mod author.

BTW is merge_replace a suitable name for that function? It sounds a bit awkward.

@numberZero
Copy link
Contributor Author

Renamed the function, hope that name is OK.
Merging in a few days if there are no objections.

@numberZero numberZero merged commit 583d2c1 into master Dec 19, 2020
@numberZero numberZero deleted the fix-mergetable branch December 19, 2020 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mesecon.mergetable semantics is unclear
3 participants