| Time |
Nick |
Message |
| 02:54 |
|
wsor joined #minetest-dev |
| 04:00 |
|
MTDiscord joined #minetest-dev |
| 05:23 |
|
calcul0n_ joined #minetest-dev |
| 06:42 |
paradust |
Is there a way to expose a core function to lua so that it can be used by builtin/, but is not visible to mods / games ? |
| 06:43 |
paradust |
oh, guess I could just save it to a local and nil the global |
| 07:16 |
sfan5 |
thats the way yes |
| 09:07 |
|
appguru joined #minetest-dev |
| 09:16 |
MTDiscord |
<luatic> That's one way. A cleaner way would probably passing a table with functions private to builtin around when loading files. |
| 09:16 |
MTDiscord |
<luatic> probably be* |
| 10:03 |
|
behalebabo joined #minetest-dev |
| 10:04 |
|
HuguesRoss joined #minetest-dev |
| 11:35 |
|
Fixer joined #minetest-dev |
| 14:23 |
appguru |
My serialization redo PR removed a check that would silence incorrect types being passed to minetest.deserialize, which is now triggering bugs in poorly written mod code: https://github.com/runsy/petz/issues/105#issuecomment-1153975021. Note that the handling of non-string types was not documented (and I consider throwing clean here). Should I make a PR to preserve backwards compat at all costs? |
| 14:24 |
appguru |
What about a PR that would only preserve backwards compat for `nil` strs? Passing userdata, tables, numbers, functions or bools to minetest.deserialize should surely throw? |
| 14:24 |
|
Taoki joined #minetest-dev |
| 14:26 |
appguru |
https://github.com/runsy/petz/issues/105#issuecomment-1153991391 alright, seeing as runs is passing a table, this is clearly a bug in his code which the redo just surfaced |
| 14:26 |
appguru |
I thus maintain that this increase in strictness (rather than possibly unexpected behavior) was for the better. |
| 14:27 |
erle |
appguru did this show a warning before? |
| 14:27 |
appguru |
erle: no |
| 14:27 |
appguru |
I would add a warning and a note in the docs if I were to restore backwards compat |
| 14:27 |
erle |
if runs code worked without a warning, randomly breaking it is a bit of a dick move, even if it is arguably incorrect. |
| 14:28 |
appguru |
erle: the code was broken |
| 14:28 |
appguru |
he is deserializing twice |
| 14:28 |
appguru |
my PR surfaced the bug |
| 14:28 |
appguru |
so that's a good thing |
| 14:28 |
erle |
i'd add a warning nevertheless, then make it strict some time later. |
| 14:28 |
appguru |
because his persistence never worked and he probably didn't even know it |
| 14:28 |
erle |
but you have to get the warning entirely correct (i.e. if it does not warn, the code must be correct) |
| 14:29 |
erle |
if it works as well as the warning about transparency in textures you'll have no problems making it super strict a year later |
| 14:30 |
erle |
given that people accidentally rely on all kinds of bullshit without realizing it, giving them a head start seems like good API stewardship |
| 14:30 |
erle |
unless there is a security issue involved, then make it as tight as possible obv |
| 14:32 |
appguru |
erle: runs has fixed the bug now |
| 14:32 |
appguru |
I wonder whether he would've bothered fixing a warning spam? |
| 14:38 |
erle |
no idea. but the only warning i have seen fail is the one about dynamic media – and the reason was that it was phrased incorrectly (no mention of blocking vs non-blocking) and a wrong fix made the warning disappear (an empty callback function). |
| 14:38 |
erle |
everything else leads to people sooner or later fixing it. more later than sooner. |
| 14:38 |
erle |
appguru any cases besides runs case? |
| 14:38 |
appguru |
erle: no, not yet |
| 14:39 |
erle |
is there a unit test covering that code path? |
| 14:40 |
erle |
i.e. calling the function with nil? |
| 14:40 |
MTDiscord |
<luatic> erle: no, and I certainly won't add one |
| 14:40 |
erle |
why not? almost all tests in real-world software concern the unhappy path |
| 14:41 |
erle |
also that way you could make sure it does not get enabled again in a future refactoring |
| 14:43 |
MTDiscord |
<luatic> anyways, here's an (untested) patch |
| 14:43 |
MTDiscord |
<luatic> https://cdn.discordapp.com/attachments/747163566800633906/985917438153592852/Restore_deserialize_backwards_compat.patch |
| 15:50 |
Pexin |
note about petz: last week(?) I was trying to repro a thing (and failing) but t |
| 15:50 |
Pexin |
pony's death func crashed to menu |
| 15:51 |
Pexin |
on master |
| 15:51 |
Pexin |
had to go back to 5.5.0 |
| 16:11 |
appguru |
Pexin: That's exactly the bug we were talking about. Runs has since fixed it. |
| 16:12 |
Pexin |
ah |
| 16:24 |
sfan5 |
merging #12422, #12428 in 5m |
| 16:24 |
ShadowBot |
https://github.com/minetest/minetest/issues/12422 -- Fix permissions on workflow-generated macOS builds by sjml |
| 16:24 |
ShadowBot |
https://github.com/minetest/minetest/issues/12428 -- Improve lua_api.txt a bunch by sfan5 |
| 16:28 |
appguru |
sfan5: while you're at it you could merge #12423 |
| 16:28 |
ShadowBot |
https://github.com/minetest/minetest/issues/12423 -- Docs: add missing backticks by Zughy |
| 16:29 |
appguru |
it's trivial too and the two other missing backticks were fixed as well now :) |
| 16:29 |
sfan5 |
I will if it doesn't become conflicted after I merge mine |
| 16:31 |
appguru |
good |
| 16:31 |
appguru |
chances that it conflicts are pretty low if you ask me |
| 16:32 |
sfan5 |
oh right I thought I added an identical missing backtick, but I didn't |
| 17:15 |
MTDiscord |
<exe_virus> Appguru should write up a lua linter for us to run on the lua_api.txt |
| 17:16 |
MTDiscord |
<exe_virus> Seems like the kind of person that might enjoy it |
| 17:17 |
MTDiscord |
<luatic> nah, MD linting is a pesky topic :P this backtick thing was easy to approximate decently tho |
| 17:20 |
MTDiscord |
<exe_virus> Right, just do the low hanging fruit, better than nothing |
| 17:20 |
MTDiscord |
<exe_virus> Could be in any language really |
| 17:20 |
MTDiscord |
<exe_virus> Just thinking aloud |
| 17:58 |
sfan5 |
what was the policy on feature checks again |
| 17:58 |
sfan5 |
it's impossible to detect the presence of set_lighting without getting ahold of a player object |
| 18:02 |
Pexin |
are conflicts automatically flagged in existing PRs, or only when commits are pushed to them? |
| 18:02 |
sfan5 |
flagged = ? |
| 18:03 |
Pexin |
like, the red X appears |
| 18:03 |
sfan5 |
yes |
| 18:03 |
Pexin |
ok |
| 18:03 |
sfan5 |
the 'Rebase needed' label is manual labor |
| 18:04 |
sfan5 |
@Luatic: http://sprunge.us/A2LpEc?diff look good? |
| 18:24 |
MTDiscord |
<luatic> Hmm yes, I guess so. I'd probably shorten this to assert(ItemStack"".add_wear_by_uses, "errmsg") but it's fine either way. |
| 18:26 |
sfan5 |
pushing then |
| 18:33 |
|
HuguesRoss joined #minetest-dev |
| 18:34 |
|
Yad joined #minetest-dev |
| 19:18 |
|
Noisytoot_ joined #minetest-dev |
| 19:39 |
sfan5 |
are set_properties calls cached yet |
| 19:40 |
sfan5 |
hmm no |
| 22:33 |
|
panwolfram joined #minetest-dev |
| 22:34 |
|
Pexin joined #minetest-dev |
| 23:13 |
|
AliasStillTaken joined #minetest-dev |
| 23:19 |
|
wsor joined #minetest-dev |
