mt-mods / travelnet

Network of teleporter-boxes that allows easy travelling to other boxes on the same network. (Mod for Minetest)
https://content.minetest.net/packages/mt-mods/travelnet/
GNU General Public License v3.0
7 stars 12 forks source link

Consistently rename owner to owner_name (except metadata prop); Move a line below a condition to avoid crash. #56

Closed oversword closed 2 years ago

oversword commented 2 years ago

It is currently impossible to set the owner when creating a travelnet because of a confusion with naming conventions.

All instances of owner have now been changed to owner_name, except for the metadata property of the travelnet, which should not be changed for backward-compatibility reasons.

We also ran into a crash recently: 2022-06-09 10:52:06: ERROR[Main]: ServerError: AsyncErr: ServerThread::run Lua: Runtime error from mod 'travelnet' in callback on_playerReceiveFields(): ...e/billys/bls/bin/../mods/travelnet/on_receive_fields.lua:143: attempt to index local 'props' (a string value)

Which is caused by accessing props as if it's a table when the validation has actually failed, I just moved that line below the condition to fix this.

oversword commented 2 years ago

Ooh I forgot I had merge power, will leave until tomorrow in case someone objects

oversword commented 2 years ago

While I would prefer "owner" over "owner_name" it is matter of taste for internal stuff so fine for me.

Yeah, I am fine with "owner" as an outward facing thing, that's how it is in the error messages and such, but I think owner_name is better internally because of clarity.

Also all that matters is it works! And it's too late to go and change it all to "owner" now unless you want to do it :P

S-S-X commented 2 years ago

find . -type f -iname '*.lua' | xargs sed -ri 's/owner_name/owner/g' 💥

Yeah.. maybe not

oversword commented 2 years ago

I guess we have linting so it would catch any duplicates that may cause (e.g. owner = minetest.get_player(owner_name))... but then its the effort to resolve them

oversword commented 2 years ago

Will merge now I have two thumbs, will take the preference of simply owner into consideration in the future, but would like to merge this as I have another update to do right now as well

oversword commented 2 years ago

RE: owner, we also have station_name and station_networkwhich could just be station and network, maybe we could think about a new naming structure in general? @S-S-X @Panquesito7