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

To s or not to s, that is the question #14372

Open
appgurueu opened this issue Feb 15, 2024 · 6 comments
Open

To s or not to s, that is the question #14372

appgurueu opened this issue Feb 15, 2024 · 6 comments
Labels
Bug Issues that were confirmed to be a bug @ Documentation good first issue

Comments

@appgurueu
Copy link
Contributor

appgurueu commented Feb 15, 2024

Summary

I ran across this while working on BuckarooBanzay/luacheck#1.

Our docs say

Registered callback tables

All callbacks registered with [Global callback registration functions] are added to corresponding minetest.registered_* tables.

But our code does

core.registered_on_chat_messages, core.register_on_chat_message = make_registration()
core.registered_on_chatcommands, core.register_on_chatcommand = make_registration()
core.registered_globalsteps, core.register_globalstep = make_registration()
core.registered_playerevents, core.register_playerevent = make_registration()
core.registered_on_mods_loaded, core.register_on_mods_loaded = make_registration()
core.registered_on_shutdown, core.register_on_shutdown = make_registration()
core.registered_on_punchnodes, core.register_on_punchnode = make_registration()
core.registered_on_placenodes, core.register_on_placenode = make_registration()
core.registered_on_dignodes, core.register_on_dignode = make_registration()
core.registered_on_generateds, core.register_on_generated = make_registration()
core.registered_on_newplayers, core.register_on_newplayer = make_registration()
core.registered_on_dieplayers, core.register_on_dieplayer = make_registration()
core.registered_on_respawnplayers, core.register_on_respawnplayer = make_registration()
core.registered_on_prejoinplayers, core.register_on_prejoinplayer = make_registration()
core.registered_on_joinplayers, core.register_on_joinplayer = make_registration()
core.registered_on_leaveplayers, core.register_on_leaveplayer = make_registration()
core.registered_on_player_receive_fields, core.register_on_player_receive_fields = make_registration_reverse()
core.registered_on_cheats, core.register_on_cheat = make_registration()
core.registered_on_crafts, core.register_on_craft = make_registration()
core.registered_craft_predicts, core.register_craft_predict = make_registration()
core.registered_on_protection_violation, core.register_on_protection_violation = make_registration()
core.registered_on_item_eats, core.register_on_item_eat = make_registration()
core.registered_on_item_pickups, core.register_on_item_pickup = make_registration()
core.registered_on_punchplayers, core.register_on_punchplayer = make_registration()
core.registered_on_priv_grant, core.register_on_priv_grant = make_registration()
core.registered_on_priv_revoke, core.register_on_priv_revoke = make_registration()
core.registered_on_authplayers, core.register_on_authplayer = make_registration()
core.registered_can_bypass_userlimit, core.register_can_bypass_userlimit = make_registration()
core.registered_on_modchannel_message, core.register_on_modchannel_message = make_registration()
core.registered_on_player_inventory_actions, core.register_on_player_inventory_action = make_registration()
core.registered_allow_player_inventory_actions, core.register_allow_player_inventory_action = make_registration()
core.registered_on_rightclickplayers, core.register_on_rightclickplayer = make_registration()
core.registered_on_liquid_transformed, core.register_on_liquid_transformed = make_registration()
core.registered_on_mapblocks_changed, core.register_on_mapblocks_changed = make_registration()

(the attentive reader may have spotted a subtle difference: some of our registered_* tables have an s suffix, like registered_on_placenodes, while others do not, like registered_on_priv_grant; there seems to be no consistent rule being followed here)

Steps to reproduce

Access minetest.registered_*s vs minetest.registered_*, or just 👁️ 👄 👁️

What we can do

  1. Create aliases for the -s tables without the s to match what the docs say.
  2. Deprecate the -s tables.

(or the other way around)

Other suggestions?

@appgurueu appgurueu added Bug Issues that were confirmed to be a bug @ Documentation good first issue labels Feb 15, 2024
@rubenwardy
Copy link
Member

rubenwardy commented Feb 15, 2024

For the most part, it looks like s is added if the thing ends in a noun, and no s otherwise. But there's the exception of registered_on_generateds

Proposal

I'd say no s for callbacks, and an s for object types like registered_items.

registered_globalsteps should also become registered_on_globalstep, so all callbacks are on_*, with the register function matching as well

@Desour
Copy link
Member

Desour commented Feb 15, 2024

Another Proposal

Prefer consistency. core.register_<thing> -> core.registered_<thing>s, always.

One exception though: If only one can be registered (e.g. register_authentication_handler), it's singular core.registered_<thing>.

(And obviously add aliases for the old names.)

@rubenwardy
Copy link
Member

My issue with that is that registered_on_generateds does not make grammatical sense, "generated" isn't a thing that can be plural

@alerque
Copy link

alerque commented Feb 16, 2024

If you want everything to be grammatical I think you'd need to change the scheme entirely and use registered_*_hooks instead of registered_*s. Some projects and programmers care about natural language grammar more than others, some even prefer the less linguistic/natural more technical/mechanical approach.

Not my circus not my monkeys, I just came here as the maintainer of luacheck looking to check if there was an official stance and/or community consensus and what linter rules should be canonical.

@fluxionary
Copy link
Contributor

My issue with that is that registered_on_generateds does not make grammatical sense, "generated" isn't a thing that can be plural

it makes grammatical sense if you read the "on_generated" part in the quotative sense, as in "the registered 'on_generated' callbacks", similar to how "there are two 'the's in the sentence 'the man went in the house'" is grammatical even though 'the' isn't a noun and can't be plural.

that said, i'm not in favor of changing the status quo - other than documenting what's already there - because it'll mostly be a headache for mod maintainers with very little practical use except for not annoying pedants.

of the two existing proposals, i prefer Desour's.

@grorp
Copy link
Member

grorp commented Mar 31, 2024

I agree with fluxionary that it doesn't make much sense to rename all these tables just to have consistent "s". We'll have to keep the old names anyway for backwards compatibility, and then we'll have two names for the same thing, with the only difference being an "s" at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues that were confirmed to be a bug @ Documentation good first issue
Projects
None yet
Development

No branches or pull requests

6 participants