You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
In the documentation, there is the deprecated functionminetest.get_mapgen_params() which lists several options of mapgen params that can be retrieved from that method, and tells users to use minetest.get_mapgen_setting(name) instead. However, minetest.get_mapgen_setting does not have any information about what needs to be passed for name. It is easy to assume that it is the same list of parameters as listed in minetest.get_mapgen_params. For most of the params this is true, but the deprecated function uses mgname and the new function uses mg_name. One would have to know to look through map_meta.txt to search for something similar to mgname to know what to pass to minetest.get_mapgen_setting, since there is no information correcting this in the docs yet.
While the docs are technically correct, it is far too easy to assume the name is the same between two functions that do the same thing from the user's perspective. There is also no mention of mg_name anywhere in the docs, but there is a mention of mgname, meaning one is more likely to assume a documented version of the API is correct, and the non-documented one is a typo or purely an internal name like in map_meta.txt for example.
Solutions
Since deprecating the function is the goal, it would be good to make it clear that it's not just a broken function or that the docs are false, and instead that the new function is not a perfect 1:1 replacement for the deprecated one.
The obvious fix is to add a list of the more common params available to minetest.get_mapgen_setting, including the param name which has the discrepancy mg_name. This would mean users would look up the new function, see that the name is different and correct it, instead of assuming the documented (old) name is still being used in the new function.
For example:
* All string and number settings contained in map_meta.txt are available such as:
* `mg_name`
* `seed`
* `chunksize`
* `water_level`
* `mg_flags`
Another option would be to detect mgname as a parameter and show a warning, or automatically correct to mg_name.
WARNING[Main]: minetest.get_mapgen_setting uses different setting names than minetest.get_mapgen_params: mgname is deprecated, use mg_name
However, with flags --> mg_flags also being different it doesn't seem like a very elegant solution to the problem, and the previous option seems preferable. Perhaps a general warning such as "setting with name [name] not found" would be better for this type of solution, but that doesn't solve the problem of not knowing the correct name to use.
Alternatives
The alternative is to just have to have people occasionally be confused and have to be corrected or have them potentially switch back to the deprecated function out of frustration.
Additional context
No response
The text was updated successfully, but these errors were encountered:
The obvious fix is to add a list of the more common params available to minetest.get_mapgen_setting
Would you like to open a PR to address this issue directly? I am sure with direct suggestions in form of changed lines would be optimal to discuss the exact wording. It also makes sure that nothing is forgotten from what you intended to what's effectively changed.
Perhaps a general warning such as "setting with name [name] not found"
minetest.get_mapgen_setting already returns nil when no such setting exists, thus I don't think such warning is necessary.
Afterthought. in the long run it might make sense to use the Settings (Lua) object for map_meta.txt as well and deprecate all specialized functions related to mapgen settings.
Problem
In the documentation, there is the deprecated function
minetest.get_mapgen_params()
which lists several options of mapgen params that can be retrieved from that method, and tells users to useminetest.get_mapgen_setting(name)
instead. However,minetest.get_mapgen_setting
does not have any information about what needs to be passed forname
. It is easy to assume that it is the same list of parameters as listed inminetest.get_mapgen_params
. For most of the params this is true, but the deprecated function usesmgname
and the new function usesmg_name
. One would have to know to look throughmap_meta.txt
to search for something similar tomgname
to know what to pass tominetest.get_mapgen_setting
, since there is no information correcting this in the docs yet.While the docs are technically correct, it is far too easy to assume the name is the same between two functions that do the same thing from the user's perspective. There is also no mention of
mg_name
anywhere in the docs, but there is a mention of mgname, meaning one is more likely to assume a documented version of the API is correct, and the non-documented one is a typo or purely an internal name like inmap_meta.txt
for example.Solutions
Since deprecating the function is the goal, it would be good to make it clear that it's not just a broken function or that the docs are false, and instead that the new function is not a perfect 1:1 replacement for the deprecated one.
The obvious fix is to add a list of the more common params available to
minetest.get_mapgen_setting
, including the param name which has the discrepancymg_name
. This would mean users would look up the new function, see that the name is different and correct it, instead of assuming the documented (old) name is still being used in the new function.For example:
Another option would be to detect
mgname
as a parameter and show a warning, or automatically correct tomg_name
.However, with flags --> mg_flags also being different it doesn't seem like a very elegant solution to the problem, and the previous option seems preferable. Perhaps a general warning such as "setting with name [name] not found" would be better for this type of solution, but that doesn't solve the problem of not knowing the correct name to use.
Alternatives
The alternative is to just have to have people occasionally be confused and have to be corrected or have them potentially switch back to the deprecated function out of frustration.
Additional context
No response
The text was updated successfully, but these errors were encountered: