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

lua_api.md: confusing information about argument passed to minetest.get_mapgen_setting #14522

Open
SumianVoice opened this issue Apr 6, 2024 · 1 comment
Labels
@ Documentation Feature request Issues that request the addition or enhancement of a feature

Comments

@SumianVoice
Copy link

Problem

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

@SumianVoice SumianVoice added the Feature request Issues that request the addition or enhancement of a feature label Apr 6, 2024
@SmallJoker
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Documentation Feature request Issues that request the addition or enhancement of a feature
Projects
None yet
Development

No branches or pull requests

3 participants