Time  Nick       Message
11:20 nerzhul    merging #8218
11:20 ShadowBot  https://github.com/minetest/minetest/issues/8218 -- Don't regain breath while in ignore node by Wuzzy2
11:28 nerzhul    pushing a lint fix too
17:33 Wuzzy      Sorry. :-(
17:34 Wuzzy      I thought paramat etc. would prevent it from being merged...
17:34 Wuzzy      Now. How do clean up the mess I caused?
19:39 sfan5      merging #8181 in 5 minutes
19:39 ShadowBot  https://github.com/minetest/minetest/issues/8181 -- Fix coloured fog in main menu by random-geek
19:51 Wuzzy      Is there a rule that says that code submissions to the Lua API will only be accepted when they are also documented?
19:51 Wuzzy      because if not, that might explain why the documentation always has holes
20:04 rubenwardy That's an implicit rule
20:04 rubenwardy There's also no explicit rule on whether a PR should actually work
20:04 rubenwardy Or an explicit rule that code should be LGpl licensed
20:07 nerzhul    Wuzzy it's implicit i think but may be added to the dev wiki i think
20:07 nerzhul    for the documentation i think it should be clear :s
20:08 Wuzzy      so... in other words, you do not accept PRs with missing documentation?
20:08 Wuzzy      if so, that's good
20:09 Wuzzy      because it seems that some undocumented functions managed to sneak in regardless... but luckily, that seems to be the exception
20:26 paramat    Wuzzy no problem, i didn't mention the lint failure because it was unrelated to your PR :)
22:00 paramat    the RTT PR needs attention if anyone can
22:01 * cheapie  sits down and pets the PR or something
22:07 Wuzzy      paramat: make it blocker then? :D
22:07 Wuzzy      oh.
22:08 cheapie    It seems like there's a lot of back and forth about some random minor stuff that needs changed. Why can't a core dev just change that so we don't have to wait around so much?
22:08 VanessaE   paramat: merge it as-is, then immediately open up an issue for that bit of code that isn't needed.
22:08 VanessaE   because it WORKS NOW.
22:09 VanessaE   cheapie's right.
22:10 cheapie    I mean, I'm not sure how the MT dev policies are, but if someone submitted a PR for my software that I liked except for one minor thing, I'd just merge it and then change that.
22:11 paramat    i need to look at the PR and see what the hold up is. also, 'works' is not good enough, code has to be good
22:12 cheapie    I think GitHub even lets you suggest changes in a review these days too. Would be a lot more useful than "The second statement does not do anything due to an unused local variable"
22:13 VanessaE   ...
22:13 cheapie    I mean, I don't have much Cwhatever experience, but I have a very hard time figuring out what SmallJoker is saying should be changed - and to what.
22:13 rubenwardy it should just be a straight revert
22:14 rubenwardy the only comments should be if there is a rebase mistake
22:14 rubenwardy not sure how experienced ANAND is with git, so that may be likely
22:15 paramat    will look at it. smalljoker has reviewed in depth so in the current absence of any other in-depth reviews it's a good idea to wait for their +1, they'll be active tomorrow
22:15 sfan5      a straight revert might not be possible because some time has passed between the rtt commit being merged
22:15 cheapie    This might even be one of those things where some code wasn't quite up to standards, and then now that someone is doing something nearby it's suddenly a problem.
22:16 cheapie    This happened last time I submitted a PR to technic - I touched one thing in one file and now suddenly all of the code style issues in that file were somehow my problem :P
22:16 cheapie    (Yes, I know technic isn't the engine)
22:17 sfan5      paramat: the comment by pgimeno definitely needs addressing
22:17 sfan5      the line he commented on is a no-op, I doubt it was this way before the rtt "fix"
22:18 paramat    ok will look at that
22:18 cheapie    Those lines seem to have been unchanged: https://github.com/minetest/minetest/blob/07b1743d3db086f0f984968252d9e3ac71336a7e/src/network/connectionthreads.cpp#L1176
22:23 sfan5      paramat: there, i fixed the pr
22:23 sfan5      you can merge it now
22:25 paramat    ok checking
22:25 paramat    thanks
22:26 cheapie    Testing it here, whenever it finishes downloading...
22:26 paramat    just found another mistake
22:26 cheapie    (yay for slow connections)
22:26 paramat    see my latest line comment
22:28 paramat    oh, nevermind
22:30 cheapie    OK, finally got a copy, now I can test it :P
22:32 cheapie    Seems good as far as I can tell *shrug*
22:33 VanessaE   applying to my servers and client, on top of clean HEAD (just now)
22:39 paramat    i'll try to review and merge it tonight
22:40 VanessaE   paramat: no offense, man, but I think you've done enough reviewing....
22:40 VanessaE   :P
22:40 VanessaE   (of this PR anyway)
22:43 paramat    no, i haven't reviewed it at all, and should have done so much sooner
22:44 VanessaE   you know what I mean.
22:57 paramat    no i don't, what else can it mean?
22:59 VanessaE   all your commentary about what is or is not needed.
22:59 paramat    apart from spotting 2 mistakes i haven't looked at it at all
22:59 VanessaE   to me, that's as much of a "review" as going through the final PR.
23:00 VanessaE   anyway, that doesn't matter.
23:00 sfan5      the question isn't really about the code since that's just a straight revert
23:00 VanessaE   sfan approved it, cheapie and I tested it.  why does it need anything more?
23:00 sfan5      the question is whether the revert should happen
23:00 sfan5      which I'm pretty sure was already decided
23:00 paramat    that's not a complete review. now please don't encourage sloppiness and rule breakage, you don't like bugs after all
23:01 cheapie    Well, the old code worked and the new code doesn't, so I think it should either be fixed or reverted :P
23:01 paramat    PRs require 2 approvals, it's not really trivial as it isn't an auto-revert, code mistakes might have happened, and in fact did happen
23:01 VanessaE   (where "old" and "new" are relative to clean HEAD, without the PR :P )
23:02 paramat    yes revert is best i think
23:23 paramat    +1 merging
23:24 paramat    (8187 that is) once checks complete
23:24 paramat    then RC2 tomorrow for, i suggest, a week of testing before release
23:26 paramat    only very minor blockers remain
23:29 paramat    LOL, now LINT is complaining about the formatting of 'clang-format on/off' lines we added to keep it happy. it just gets more ridiculous
23:30 paramat    anyway, merging
23:40 VanessaE   thanks, paramat.
23:41 cheapie    \o/
23:42 paramat    woohoo
23:43 * cheapie  accidentally runs make -j176 and promptly runs out of RAM
23:43 VanessaE   servers updated.
23:50 paramat    thanks for testing
23:50 paramat    rubenwardy +1 for #8162 can i merge it?
23:50 ShadowBot  https://github.com/minetest/minetest/issues/8162 -- Update credits by rubenwardy
23:50 rubenwardy well, it's missing translators
23:51 rubenwardy It might be worth adding a new section for that
23:54 paramat    ok
23:55 paramat    i might do some lua_api.txt improvements now as there are so many needed