Time Nick Message 02:19 hmmmm seems like a really off day 02:19 hmmmm paramat, are you around? 02:50 paramat hiya hmmmm 02:51 hmmmm in that one PR about the cave optimizations, you mentioned something about giving yourself an approval 02:51 hmmmm i said that goes against the whole point of code reviewing and then i got egged by several others. what's up with that? 02:52 hmmmm did something happen in some kind of official rulebook while i was gone that i'm not aware of? 02:52 paramat hm, we gave up on the subsystem maintainer thing 02:53 paramat there was some discussion about whether a core dev author's approval counted 02:53 paramat it seems most thought yes 02:54 hmmmm it's not about approval though 02:54 paramat but hey, we can change the rules =) 02:54 hmmmm it's about code reviewing 02:54 hmmmm are these "rules" written down in some place on a wiki page i'm not aware of? 02:54 paramat i wrote some more in that thread with my thoughts 02:55 paramat no they're not, all we have is the '2 core dev agreement' thing 02:55 hmmmm that's ancient and we changed it a while ago 02:55 * VanessaE grabs the box of eggs from the fridge... 02:55 hmmmm the number of agreements required varies based on the complexity of the change 02:55 paramat it seems the issue has arisen because i added 'approval' labels 02:56 hmmmm like for example with farmap, does it seem legitimate if nerzhul and blockmen "reviewed" it and then +1ed it in the PR and then merged it? 02:56 hmmmm or no not even 02:56 hmmmm just nerzhul and celeron's own approval 02:56 paramat good point 02:56 hmmmm and this huge sweeping complex change gets added and breaks everybody elses' PRs 02:56 hmmmm and on the other hand 02:56 hmmmm does it really require 2 people agreeing on fixing a tiny bug 02:57 paramat indeed we have trivial exception 02:57 hmmmm so first of all i think there needs to be some formalized clarification 02:57 hmmmm there is a difference between agreeing with a feature and reviewing the code for that feature 02:57 paramat yes 02:57 hmmmm perhaps that should be split 02:57 hmmmm i am a strong believer in code review 02:58 hmmmm it's caught big potential mistakes lots of times 02:58 hmmmm but many times there are approvers who don't get that and they take it for granted, give the concept an approval and +1 it 02:58 hmmmm that's not adding any value 02:59 hmmmm so what i propose is that there should be two types of approvals, concept approval and code review approval 02:59 hmmmm code review approval should scale along with code complexity like i said before 02:59 hmmmm i'm not sure how concept approval would scale 02:59 hmmmm do you agree with this so far? 03:00 paramat yes 03:00 hmmmm there's this guy i work with who's buddies with this other developer 03:00 hmmmm they keep approving eachothers commits without actually looking at the code 03:01 hmmmm "oh i trust so and so" 03:01 hmmmm blatant syntax errors that make the thing fail to compile get approved 03:01 hmmmm it's essential that code reviews are actually code reviews 03:01 paramat heh i've done that =/ 03:03 hmmmm so incredibly annoying that Kevin's shit commit breaks everything because somebody gave approval and merged it based off of "trust" 03:03 hmmmm at least minetest is a project that i have influence over, so i'd really like that to not happen here 03:03 paramat the use of 'approval' labels will have to be rethought 03:04 hmmmm now, I can't unilaterally make the rules 03:04 paramat perhaps changed to 'code approval' 03:05 hmmmm i'm not going to have some kind of democratic vote where 2/3rds of the voters don't actually bother understanding the details of what they're voting for, and another handful don't care to begin with 03:05 hmmmm but i'd like more opinions for logical soundness and potential pitfalls 03:05 hmmmm now, how do you think concept approval should scale? 03:06 paramat hmm .. 03:06 hmmmm a really simple change could be extremely controversial and should take many approvals 03:06 hmmmm whereas something highly technical probably won't have any controversy 03:06 hmmmm in general I feel like minetest is a victim of bikeshedding too often 03:14 paramat so far i feel concept approval would scale similarly 03:15 paramat i'm beginning to feel it might be good to remove my 'approval' labels now 03:15 paramat either for something new or for a more flexible scaled approach 03:16 paramat ok i'll move the unused code to the end of file 03:18 hmmmm well that was just a suggestion 03:18 hmmmm if it really is better to delete it, then fine, but i've found that deleting #if 0'd code is good for forgetting code history among other things 03:20 paramat it won't bother me at file end. unless you have a better place to store it. is addTopNodes for the old mgv7? 03:20 hmmmm yeah 03:20 hmmmm addTopNodes I'd say is less valuable to keep than carveRivers 03:20 paramat agreed 03:21 hmmmm generateBiomes seems to do the same thing as addTopNodes with more reliability 03:21 paramat this unused code could be useful for other mapgens 03:22 paramat it should perhaps be preserved somewhere 03:22 hmmmm i have a code junkyard file 03:22 hmmmm it's not publically shared though 03:23 paramat ok for now i'll keep both and move to file end 03:23 hmmmm maybe there should be a junkyard.cpp or similar that has nothing but unused code 03:23 hmmmm snippets 03:24 hmmmm but then we'd get people criticising it saying "that's what version control is for" 03:25 paramat seems good to me 03:26 hmmmm iunno keep it at the end of the file for now 03:26 hmmmm i don't feel like making an immediate decision on it 03:26 paramat sure agreed 03:27 paramat after release i'd like to make biome noises per-mapgen 03:27 paramat as mapgens need differing scales of biome 03:27 hmmmm wait 03:27 hmmmm before you do that i want to come up with a modular, flexible way of approaching this 03:28 hmmmm i know how you'll implement it and it'll suck 03:28 paramat ok, heh 03:28 hmmmm you should work on finishing your existing PRs imo 03:30 paramat yeah, i'm super-busy with stuff for release 03:30 paramat that's why simple deco rotation is being delayed 03:31 hmmmm simple deco rotation can be finished in 5 minutes 03:31 hmmmm why not just do it 03:35 paramat i need to work out how to do what was agreed, some of it is new stuff for me 03:42 sofar time to upgrade, reboot, etc. 03:51 paramat #3959 updated 03:51 ShadowBot https://github.com/minetest/minetest/issues/3959 -- Mapgen: Optimise cave noises and tunnel excavation by paramat 04:54 hmmmm Hello Zeno` 05:19 Zeno` Hi, how are you? 05:40 hmmmm I am feeling okay. 05:40 hmmmm How are you on this glorious evening, comrade? 05:49 Zeno` pretty good. People keep calling me on the telephone which is unusual :/ 06:30 celeron55 hmmmm: the solution is not more rules, but more common sense 06:31 hmmmm if there aren't rules, people will start getting ridiculous i think 06:32 celeron55 our core devs aren't just random people; i don't allow people with no ability to reason to become core devs 06:32 hmmmm except when you do 06:33 celeron55 are you referring to like one case that just pissed you off exceptionally that even i admit was a mistake? 06:35 hmmmm possibly 06:35 hmmmm i'm just saying it can happen 06:36 celeron55 you're basically threatening to become ridiculous yourself if there aren't very strict rules for everyone; i think this is rude and a waste of time 06:36 hmmmm i'm not saying strict rules 06:36 hmmmm everything is ultimately negotiable 06:38 hmmmm rules are worthwhile because they set a semi-objective standard to operate based on 06:39 hmmmm if they didn't exist, i think there would be much more bickering about decisions that weren't popular with somebody 06:41 celeron55 there are enough rules; everything has went just fine until you suddenly became active again and decided that it shouldn't be the case 06:42 sofar if the rules are never broken, they are as useless as rules that are never followed 06:43 hmmmm hey the only reason why i brought up anything about rules is because i started getting people yelling at me for 'not following the rules' 06:43 hmmmm not to mention, what i proposed isn't adding more, it's modifying what's existing 06:44 celeron55 what exactly happened in "the" PR then? 06:44 hmmmm i don't know 06:44 celeron55 like, what was your criteria that resulted in you approving it just by yourself 06:44 hmmmm est31 blew a gasket and got very upset 06:45 hmmmm because i thought we all had an agreement that number of reviews/approvals should vary based on the complexity of the change 06:45 hmmmm but apparently some people forgot about that or didn't like it ..? 06:45 celeron55 i know that, but what's the detaul 06:45 celeron55 detail* 06:45 hmmmm i reviewed the code, recommended changes, the changes were made, i was overall satisfied with the feature and its implementation so i approved it 06:45 celeron55 when i looked at the PR, it did not look like a trivial change 06:45 hmmmm how is it not..? 06:45 celeron55 more like a medium change or something 06:46 hmmmm trivial is something like fixing a comment 06:47 celeron55 well, at least in my opinion API changes are never trivial 06:49 celeron55 is that the addition to rules that you need? 06:49 hmmmm huh? 06:50 hmmmm in any case, if this is such a waste of time, you wouldn't be bothering to talk about 06:50 celeron55 do you not understand what i said or what's the "huh?" 06:51 hmmmm i didn't understand what the "that" referred to 06:51 celeron55 "API changes are never trivial" 06:51 celeron55 = "that" 06:51 hmmmm sure, i'll be willing to agree with that 06:51 hmmmm but let's talk nomenclature for a second 06:52 celeron55 would that have changed what you did in the PR? 06:52 hmmmm on this imaginary scale i have in my brain, trivial counts as 0, small counts as 1, medium counts as 2, etc. etc. 06:52 hmmmm i'd say that specific PR counted as small 06:53 hmmmm your sentence implies that "an API change should always have at least 2 reviewers" 06:56 celeron55 what i personally do is something like this: trivial i will merge by myself, small i will merge by one review only if it isn't me or nobody is willing to look at it for like 20 hours after i ask on IRC 06:58 gregorycu I think we also just need to have a bit of faith in the code devs. It's possible different people will consider different things to be "small" chsnges 06:58 celeron55 like, the general thing has been that you always make a honest attempt to get two reviews 06:58 celeron55 no matter how large the PR is 06:59 celeron55 if you can't get, then the strict rules are a bit loose but it happens rarely 06:59 gregorycu So don't get too aggressive when somebody doesn't seem to treat it as a big of a deal as someone else 07:00 gregorycu (in other words: basically this is all hmmmm's fault) 07:00 celeron55 it's especially bad when you look like you didn't want any input from any other core dev 07:00 celeron55 don't do things that end up you looking like that; it's a bad idea 07:01 gregorycu (I was kidding by the way) 07:01 hmmmm it was a PR uploaded on github available for review to the entireity of the world's internet users 07:01 hmmmm nobody else looked at it except for me 07:01 hmmmm seems like enough of a chance to me... 07:02 hmmmm i'm done with this discussion. it's just a waste of time. 07:02 celeron55 i think this discussion needs some kind of a finish though 07:03 celeron55 i'm not sure what would be 07:03 gregorycu I think it's fair to say that maybe there was a slight overreaction by est but there was no malice with either of hmmmm's or his actions? 07:03 hmmmm those darn germans always going on about rules and regulations 07:05 celeron55 it's not actually just est31 who didn't like it 07:05 celeron55 not to name names but still 07:05 hmmmm you? 07:05 celeron55 not me 07:05 hmmmm you seem pretty upset 07:06 gregorycu Easy lads 07:07 celeron55 or, well, i'm a bit upset, but mostly just because of this hassle and not really the PR itself 07:07 gregorycu Can't we just chalk this up to a disagreement with regards to the size of the change? 07:07 gregorycu Given the number of approvals required is flexible 07:07 celeron55 i guess that's what we have to do 07:08 hmmmm something like this is bound to happen again unless a resolution of some sort is made 07:08 hmmmm i was just giving celeron a gentle jab with the "i'm done with this conversation! such a waste of time" 07:09 hmmmm "this conversation is a waste of time" is practically celeron's catch phrase - it's ironic too, because right after saying that he gets invested into the topic 07:09 gregorycu We can avoid these issues in the future by having clear guidelines and also assuming good faith 07:10 gregorycu And by "we" I mean you core devs :) 07:11 hmmmm right, but 'guidelines' is a gentler term for 'rules' and celeron doesn't want no stinkin rules 07:11 celeron55 reviewing what was said now, it seems that one clear issue is that for all the time that hmmmm has been away, our practice has been that anything that isn't trivial (eg. small) needs two approvals 07:11 hmmmm don't you mean to use i.e. there? not e.g. 07:12 celeron55 umm... "for example" 07:12 hmmmm o.k. 07:12 hmmmm so what about trivial changes 07:13 hmmmm has that been 0 or 1 approval? 07:13 celeron55 1, which can be the one merging it or not 07:14 gregorycu Can that one also be the author? 07:14 celeron55 approving and merging a thing are separate actions 07:14 celeron55 i think yes 07:15 hmmmm that's a silly technicality 07:15 hmmmm essentially, you're telling people that it's fine to code review their own commit 07:15 celeron55 we have a defined way to do a trivial change by a core dev with nobody being involved 07:15 celeron55 it's the one where you say about it on IRC, wait for 15 minutes or something and then merge it 07:16 hmmmm right but that has 0 code reviews 07:16 gregorycu Well 07:16 celeron55 well, approval and review is a different thing 07:16 gregorycu It has 1 core dev 07:17 hmmmm in your words, what is the definition of two 07:17 celeron55 "approval by somebody else than the author" is a "review" 07:17 hmmmm of the two* 07:17 hmmmm okay that's just mincing words 07:17 hmmmm i'm sorry that's horrible 07:17 gregorycu Hang on 07:18 celeron55 i think it's very useful to be able to decide whether you approve your own thing 07:18 hmmmm is somebody going to submit their change and not approve it? 07:18 celeron55 i do 07:18 celeron55 rarely, but anyway 07:19 gregorycu Maybe if you're not sure there are problems? 07:19 gregorycu I don't know 07:19 hmmmm then mark your change as a work in progress 07:19 celeron55 basically it just scales the reviews needed for it 07:19 hmmmm i think that's a silly distinction honestly 07:19 hmmmm the act of offering the code is an implicit approval from yourself 07:19 gregorycu Can I complicate matters? 07:20 celeron55 maybe it could be changed 07:20 hmmmm never mind changed, who came up with that in the first place? 07:20 hmmmm where was it written? 07:20 gregorycu What about concept review vs code review? 07:20 hmmmm i think that's a great idea 07:20 celeron55 i think it's just what has happened in practice 07:20 hmmmm what about you, celeron? 07:21 celeron55 the only reason any of these practices have seemed to surface is due to the approval tags on github, which is kind of funny 07:21 hmmmm paramat pointed that out too 07:23 celeron55 i'm open to the idea of concept review but not sure what it would mean in practice 07:23 hmmmm perhaps review is the wrong term here, it's really an approval 07:23 hmmmm approval of what the change accomplishes 07:24 hmmmm you know all the +1s that pull requests typically get from members of the peanut gallery who aren't developers and aren't really technical to begin with? 07:24 hmmmm they aren't reviewing the code 07:24 hmmmm they like the idea of what the PR does 07:24 celeron55 if there was a separation between concept approval and code review, how many of each would be needed? 07:25 celeron55 and from who 07:25 hmmmm the current approval system was really designed with code reviews in mind 07:25 gregorycu I think there are non devs that should be able to perform concept reviews 07:25 gregorycu Or vote 07:25 gregorycu I don't know 07:26 gregorycu Maybe concept review is the issue, PR links to issue 07:26 celeron55 a full-on democracy may be a bad idea though 07:26 hmmmm that's essentially what the approval system is right now 07:26 hmmmm we have -1s and +1s 07:26 hmmmm the total score reaches a certain point 07:26 hmmmm how is that not equivalent to voting 07:27 gregorycu Issue discusses only concept. PR is only code. PR can't be checked in if the issue does not meet some requirement 07:27 gregorycu I agree that democracy is bad 07:27 celeron55 the votes by non core devs don't require any specific reaction by anyone 07:28 gregorycu What is special about core devs? 07:28 hmmmm core developers still vote for the concept and not the code review, at times 07:28 gregorycu There are some non-devs that could be trusted to help decide things 07:28 celeron55 gregorycu: if it's a PR, from a core dev it's this review/approval/whatever thing 07:30 gregorycu People that have been around for ages, big mod writers could very much contribute to concept review 07:30 gregorycu For the record, I'm not asking to be one of these people. 07:30 celeron55 concept review extends to technical feasibility on the engine side though 07:31 hmmmm i agree with the sentiment but it's hard to implement it, gregory 07:31 celeron55 some things aren't possible even if people want them and they would have a reasonable API 07:31 gregorycu Right. Obviously if a dev says "it's not possible" in the concept review that has a big of weight 07:31 gregorycu Bit 07:31 celeron55 like in the case of requiring a bloated library 07:32 gregorycu I don't think the concept review should be this scaler thing where you hit +3 and it's in 07:32 hmmmm my idea of concept approval is everything associated with an 'approval' - code reviewing 07:32 gregorycu I think with these things you keep going until everyone is reasonably happy 07:33 hmmmm the only reason why there is a separation is because people confuse having an opinion about something is as useful as having reviewed the code for mistakes or security flaws and so on 07:34 hmmmm right i don't think there needs to be any kind of post needed to be passed for a concept review 07:34 gregorycu How do we feel about non-bug-fixes requiring issues? 07:34 celeron55 officially splitting things to user votes, concept approval and code review might make sense 07:35 gregorycu As long as voting isn't binding :) 07:35 celeron55 the latter two would be core devs and concept approval could have some extra people who know their stuff 07:35 hmmmm concept approval should be consensus-driven 07:35 celeron55 voting would be completely non-binding, only to give developers pointers to what people want 07:35 gregorycu Votes are a guideline for user interest 07:35 gregorycu Hmmmm: yeah, agree 07:35 celeron55 user's needs are an important source of motivation so it should be enough from the user's point of view 07:37 hmmmm one thing i don't agree with is the idea that each type of approval should correlate to a different type of entity on github 07:37 hmmmm i.e. issue vs. pr 07:37 hmmmm it should be per-commit 07:37 celeron55 i don't agree with that either; even just because github weirdly mixes issues and PRs 07:37 gregorycu Per commit? 07:37 hmmmm per-change 07:38 gregorycu I've been assuming commits are squashed... 07:38 hmmmm let's say somebody decided to fix bug #403434 by adding some heavyweight library 07:38 ShadowBot hmmmm: Error: Delimiter not found in "HTTP Error 404: Not Found" 07:38 hmmmm that might pass code review for soundness but not concept review 07:38 gregorycu Ok 07:38 hmmmm commit squashing is another area we ought to clarify 07:39 gregorycu Probably a bit of an edge case? 07:39 gregorycu I assume the library will be visible in the code review? 07:39 hmmmm contributions should be grouped into logical sets of changes through commits 07:39 hmmmm when possible 07:40 hmmmm oh that's another thing 07:40 hmmmm code reviewing a library... 07:40 hmmmm if you just say "we don't code review libraries" you don't know what you could be slipping in 07:41 hmmmm also contributors could write the main chunk of their contribution as a third party library in an effort to sidestep unpopular regulations 07:41 gregorycu Lol 07:41 hmmmm or it might be a well intentioned attempt at modularity 07:41 gregorycu We should not be changing libraries 07:42 gregorycu People change libraries? 07:42 hmmmm you don't know 07:42 gregorycu That should be an auto code review fail 07:42 hmmmm that'd be a really great way to distribute malware to minetest users 07:42 celeron55 what should happen when a contribution includes a bundled library is that a core developer re-downloads and re-bundles it from a trusted source 07:42 celeron55 we don't have many, but one or two haven't gone through that process for sure 07:43 gregorycu (we really shouldn't be including the library with minetest) 07:43 hmmmm okay i hate to use est as the wipping boy here but 07:43 hmmmm what about cSRP 07:43 gregorycu (I know it's easier) 07:43 celeron55 gregorycu: it's reasonable for helping windows builds 07:43 gregorycu *shrug* I'm on Windows 07:44 gregorycu That's fine. But I think the hard rule is we don't change libraries 07:44 gregorycu For me, changing a library should require a concept review 07:45 hmmmm sure i agree with that 07:45 hmmmm but that never happens 07:45 gregorycu Because it can affect supported platforms etc 07:45 gregorycu Maybe 07:45 gregorycu I don't know. 07:45 celeron55 (it has happened once; with jthread; but jthread is so small it's practically not a library) 07:45 hmmmm right and what about cSRP 07:46 gregorycu What does that stand for? 07:46 hmmmm that's the name of it 07:46 hmmmm it's est's modified SRP library 07:46 celeron55 i haven't kept up with that; i trust that the ones who have worked on it know what they are doing 07:46 celeron55 and aren't malicious 07:47 celeron55 anyway, this kind of slipped to a different topic 07:49 hmmmm anyway i'd like to spend the 11 minutes i have left to merge some PRs 07:49 celeron55 separating code reviews, concept reviews and user votes <- if someone other than me and hmmmm have something to say on this, go ahead 07:49 gregorycu Yeah. You should not modify deps 07:50 hmmmm is somebody going to scream at me if i merge a pr 07:51 gregorycu Stupid question. After all this talk, are the guidelines somewhere? 07:51 hmmmm i think there are because people have been copy pasting quotes about the rules 07:51 Zeno` https://github.com/minetest/minetest/blob/master/CONTRIBUTING.md 07:52 Zeno` they are also on the minetest forum somewhere 07:52 hmmmm should -Wunused-function be disabled? 07:52 hmmmm just because a function isn't used doesn't mean it won't be used in the future 07:52 Zeno` hmmmm, good question. I don't think it should be disabled, but you'll get 50 different opinions from 50 different people :) 07:52 gregorycu C55, I also had an opinion, but I guess it doesn't count - not a core dev (I think it matches both of yours) 07:53 Zeno` If it's a useful function I'd rather #ifdef it away for a year or two and then delete it 07:53 Zeno` as long as it's not making things messy 07:54 hmmmm does anybody have an objection to bringing blit_with_interpolate_overlay back? 07:55 Zeno` oh, I mentioned on IRC that I'd have preferred that to be left in the code but #ifdef'd out 07:55 hmmmm seems kinda silly to me to remove something with obvious potential for use just to placate an overzealous compiler warning 07:56 Zeno` but didn't feel strongly either way really so didn't press the issue (I don't mind if it comes back and is #if 0 #endif'd) 07:57 Zeno` if it's not there and it's needed nobody is going to look at git history to see if it existed in the past, so I guess my opinion would be more on the side of bringing it back 07:57 hmmmm exactly 07:57 hmmmm there's this common counterargument that comes up whenever somebody #if 0's out code 07:57 hmmmm "but that's what revision control is for!" 07:57 hmmmm nobody reads the history 07:58 hmmmm unless it's needed for a very specific reson 07:58 hmmmm reason* 07:58 hmmmm might as well be completely deleted at that point 07:58 Zeno` just make sure there's a comment saying that the only reason it's #if'd away is because it's unused 07:58 Zeno` and ONLY because it's unused 07:59 Zeno` i.e. that the function is fine. in main.cpp there were a lot of #if 0 #endif's that were obviously useless and really did need to be deleted 08:02 hmmmm i hope we can all agree that adding a disabled code block qualifies as trivial 08:07 celeron55 i think #if 0 is perfectly fine in some cases 08:08 hmmmm BTW here is an example of a PR where I would give +1 code review but -1 concept approval: https://github.com/minetest/minetest/pull/3929 08:12 celeron55 i might give -1 code review but +1 concept approval, because the only reason i don't want that is because it adds so many lines of code for something nobody really uses 08:13 celeron55 if it was two lines of code, whatever 08:13 celeron55 those are kind of interchangeable 8) 08:14 gregorycu Code is cheap 08:14 gregorycu Unless it's making it complex 08:15 celeron55 code isn't cheap because all code adds complexity 08:15 celeron55 code that doesn't add complexity is usually useless 08:15 gregorycu Not all code 08:15 gregorycu But yes 08:16 hmmmm mainenance costs 08:16 hmmmm you have to test it, verify it's bug free, not a security issue, etc. 08:16 hmmmm heh here's some mapgen inspiration: http://i.imgur.com/poKTTTq.png 08:16 hmmmm i want to generate that scene 08:17 gregorycu Surely if nobody is going to use the feature, that's a - 1 for the concept 08:17 celeron55 i might consider a change that makes range adjustment always multiplication-based and just replaces the whole thing without adding all those conditionals 08:17 gregorycu If you're +1 a concept but will never be happy with any implementation that's not a nice move :) 08:18 gregorycu Cool 08:18 gregorycu Anyway. I was chatting to a core dev yesterday about concept reviews and they are of the same opinion 08:19 gregorycu That it's a good idea 08:19 celeron55 hmmmm: erosion is kind of hard to generate in an infinite mapgen 08:19 gregorycu We should have a concept review to track this idea 08:27 hmmmm i think we should use the donation money on hiring a group of middle managers 08:28 hmmmm they can help us get organized 08:29 gregorycu Lol 08:29 gregorycu What kind of middle managers do you work with 08:36 celeron55 currently the donation money is barely covering half of the cost of the VPS that i use to host the websites (among a few other smaller things) 8) 08:38 nrzkt also i think we should recruit a sells manager, a community manager, a CEO and some actionnaries 08:38 celeron55 yeah so... i can pay a monthly salary of $5 to each 08:38 celeron55 i'm sure this will go well! 08:40 gregorycu How could it not 08:43 nrzkt xD 08:44 celeron55 it's great that it does cover the hosting though; at least i'm not losing anything else than time 08:54 Zeno` can we hire a chef as well please? 08:58 nrzkt Zeno`, i prefer ansible 08:59 Zeno` sigh, but what about the *food*? 12:21 PilzAdam this is how I see (and apply) our approval system: every bit of code needs approval by 2 core devs 12:21 PilzAdam there is one exception to this rule: very trivial fixes (spelling, etc.) can be pushed by anyone 12:21 PilzAdam PRs by core devs automatically get the approval of the dev who is submitting the PR; this is to speed up the development by people who are already trusted; there still is a review, since another core dev has to approve, too 12:21 PilzAdam I noticed in practice, that not every core dev who approves a PR looks actually at the code 12:22 PilzAdam often people just approve the general idea of the PR 12:22 PilzAdam this is why at least one of the 2 approvals should be a code review 12:23 PilzAdam there seems to be this idea here, that "small" PRs only need 1 core dev approval to speed things up 12:23 PilzAdam I think this is wrong; if you consider that only 1 of the approvals needs to a code review, then getting the second one should be rather easy 12:24 gregorycu There is a second idea that concept reviews and code reviews should be two things 12:24 PilzAdam ("hey [random core dev who happens to be arround", do you generally agree with what this PR is doing? I reviewed the code already, so don't bother about that") 12:25 gregorycu Indeed. 12:26 gregorycu That's what concept reviews are meant to solve 12:26 PilzAdam gregorycu, using this terminology: a PR needs >=2 concepts reviews, and >=1 code review 12:27 PilzAdam a core dev can not review his own code; so the second approval for core dev PRs always needs to be a concept and code review 12:27 gregorycu Right. But you need to keep these counts separate 12:28 PilzAdam what also needs to be considered is actual testing of the code 12:28 gregorycu Also, which changes wouldn't you want a quorum of agreement with regards to the purpose behind the change 12:28 PilzAdam gregorycu, this is not needed if people state what they have already done in their "+1" comment 12:30 gregorycu A PR can be withdrawn, does the concept need to be re-evaluated again in the next PR 12:30 gregorycu Probably not. 12:31 PilzAdam I think this is already unnecessary detail 12:32 PilzAdam we also don't have rules for situations where some core devs actively disagree with a PR, while others agree 12:32 PilzAdam and I think it is good that we don't have rules for that 12:33 gregorycu I recommended that the discussion of merit of changes should live in issues, and discussion of code changes should live in PRs 12:34 PilzAdam gregorycu, nah, the specific API changes are rarely part of the issue, while they are subject to the concept review, not the code review 12:35 PilzAdam I don't want to propose changes here, btw 12:35 PilzAdam I'm just talking about how I see our current situation 12:48 gregorycu Sorry about that. Louts on the train. 12:49 gregorycu The API changes should be part of the issue 12:49 gregorycu I'm slightly preoccupied at the moment talk later 13:13 gregorycu Ok 13:43 celeron55 somebody can make an issue on github to talk about the approval/review thing 13:44 Zeno` I've been reading the comments so far and all I can say is that, in my opinion, more rules would be a bad thing 13:47 celeron55 i too think that most people probably don't want a more complex system, at all 13:49 nrzkt too many rules = advocate 13:50 Zeno` and also I don't think separating the concept review and code review would achieve much 13:51 Zeno` if someone doesn't agree with a concept then they say it in the comments and it might get a controversial label 13:51 Zeno` just more confusion 14:00 Fixer development is already slow as a snail / my look from outside 14:00 nrzkt Fixer, less slow than some months ago 14:01 Fixer just compare it too CDDA, it is younger but already 40000 commits %) "just saying" 14:17 gregorycu The intention isn't to make more rules 14:17 gregorycu It was about ensuring devs don't waste time doing work that is actually a feature people don't work 14:17 Zeno` gregorycu, I know, but people will probably interpret them as rules :) 14:17 gregorycu Don't want 14:17 Zeno` yep 14:18 Zeno` makes sense, but that kind of happens already doesn't it? 14:18 gregorycu And also, more clarity about what +1 means, along with allowing those that are not developers to have some what of a say 14:18 gregorycu I don't think it's too much to ask for people to justify a feature before (or while) doing the dev work 14:18 gregorycu But *shrug* I ain't the boss 14:19 Zeno` Maybe clarification in the writing would be good. I just know that when I first joined minetest dev I asked c55 about the approval process and he has always said that a dev can approve their own work and that if it's not from another dev then 2 devs must agree 14:20 est31 lol CDDA has less reliable sources on its wiki page than minetest had when it was deleted 14:20 Zeno` which seemed to work 14:20 est31 https://en.wikipedia.org/wiki/Cataclysm:_Dark_Days_Ahead 14:20 Zeno` so if you write a PR gregorycu (assuming you're not soon a core-dev) I could look at it, say ok, and ask somebody else to review it and "ok it" with me 14:20 Zeno` easy 14:21 est31 I disapprove with the concept of keeping dead code around just for the sake of it 14:21 est31 commit c3993f6604a646b42c1 14:21 est31 but lets keep it in, i dont want to cause more drama 14:21 Zeno` est31, we had a brief conversation in IRC earlier 14:21 Zeno` let me see the commit message 14:21 est31 Its easy to resurrect dead code, its all in VCS history 14:22 Zeno` est31, the argument was is that somebody probably won't look at the history 14:23 Zeno` so I'd, personally, be ok with it like the commit does... if it hung around for ages I'd then delete it (especially if it became non-functional because of other changes) 14:23 est31 but they look at file abc function def when they write some functionality in xyz? 14:23 Zeno` But as I said to hmmmm, ask 50 different people and you'll get 50 different answers :) 14:23 est31 well thats my answer 14:24 est31 not saying that it should be reverted or something, its not worth the trouble etc 14:24 Zeno` I actually don't feel *strongly* on either approach as long as the code is still functional (if used) 14:24 Zeno` which is why I didn't say much at all with the PR and approved of it 14:24 Zeno` (not adding it back, the PR where it was removed) 14:24 est31 either way its no big problem for me 14:25 Zeno` I think I even mentioned it in IRC :) Not a big problem for me either ... in cases like this 14:25 gregorycu Rules... structure... Often it can slow down dev, but it can also avoid friction, especially when it's clear what they are 14:25 gregorycu As you say, write them down somewhere 14:25 Zeno` when I was working on main.cpp there was huge amounts of stuff with #if 0/#end if around it that would no longer have even compiled... it was truly dead code and needed to be removed 14:26 est31 and for fairness sake: yes, I have discussed with hmmm that the number of approvals should depend on the feature size. But that was mostly targeted at big changes. 14:26 Zeno` gregorycu, they are written down but I agree with you that clarification could not hurt 14:26 est31 so bigger change == more reviewers 14:26 est31 and smaller change == only two. 14:26 est31 and ofc the trivial rule 14:27 Zeno` does it have to be a written rule though? I mean if something touches 96 files and has 9000 lines of changed code I would certainly not approve it just by myself and I don't know if any other dev would either 14:28 Zeno` even with 2 people I wouldn't commit it 14:28 Zeno` maybe 10 14:29 Obani 8999 lines ? 14:29 Zeno` I can see both sides of the argument, but I just think that trust should be a factor as long as the current rules are upheld 14:29 Zeno` Obani, that's different... 8999 is trivial 14:29 Obani oh ok 14:29 Zeno` =) 14:29 Obani I'll make a PR then 14:29 Zeno` lol 14:29 est31 yes even numbers of lines are giant PRs, odd numbers are trivial 14:32 gregorycu Looseish rules require a fair degree of taking things in good faith 14:33 est31 well i didnt like his style, that he reviewed the change, and then merged it without waiting for further approvals 14:33 Zeno` yeah, I don't agree with that either 14:34 nrzkt est31 i agree with you about dead code 14:38 gregorycu Is a minimum waiting period part of guidelines? 14:38 gregorycu Maybe it should be, if that's the expectation 14:38 gregorycu Yes, more rules, but maybe a few rules are better than people needlessly getting pissed off 14:39 Zeno` what I'd like to see more than new rules is some kind of informal "blog" (for want of a better word) of where things are at 14:39 Zeno` I still don't know, for example, what the deal with this feature freeze is 14:40 Zeno` I know what it's for, but I have no idea how it's progressing 14:40 Zeno` What can we do to communicate better? 14:40 Zeno` Or is that a silly idea? 14:42 Zeno` it does mean more work for somebody (probably the release manager) though :( 14:43 * Zeno` offers gregorycu an open invite 14:43 Zeno` gregorycu, if you're ever near BRI we should get together and have a beer and some food 14:48 gregorycu Last time I was in brisbane was a few years ago for Microsoft TechEd 14:48 gregorycu May have been the gold coast 14:48 gregorycu If I am, I'd love a beer in the hot weather 14:48 Zeno` :) 14:52 Calinou_ I think a development blog makes sense 14:52 Calinou_ with Jekyll, it's possible to have one on the main site 14:52 Calinou_ or we can have a forum section 14:53 est31 Jekyll has the advantage that we can discuss about it via prs 14:53 nrzkt i hate rubyfuck 14:53 jin_xi mailing list comes to mind once again 14:53 Zeno` It doesn't even really have to be a blog. Just somewhere that things are written down 14:53 jin_xi i dont like the discuss@pull request approach 14:53 rubenwardy Jekyll is awesome 14:54 Zeno` "today we did this" we're "aiming for xxxx" 14:54 est31 I dont like ruby, but its neccessary evil :) 14:54 jin_xi as it makes one write code _before_ discussing 14:54 Zeno` or "this week" 14:54 Calinou_ nrzkt, hate Ruby or not, we're still using it 14:55 Calinou_ jin_xi, we tried mailing list already, nobody used it, last time I heard 14:56 nrzkt Calinou_, where do we use ruby ? 14:56 est31 we use jekyll for the website 14:56 Calinou_ in Jekyll? which powers the main site currently 14:56 est31 because github doesnt allow something else 14:56 nrzkt Jekyll is as awesome as express for nodejs, as go natively 14:57 nrzkt est31, then it's not a choice :p 14:57 jin_xi thats why i wrote ...again :( 14:57 est31 the rust ecosystem has a special website: https://this-week-in-rust.org/ 14:57 est31 it also features "non core" activity 15:44 sofar patchwork? 15:52 celeron55 i have considered starting to write a "this week in minetest" style publication (at multiple occasions) but i have never decided to actually do it 15:52 celeron55 it's a lot to commit to when starting from writing essentially nothing 15:53 rubenwardy #2094 15:53 ShadowBot https://github.com/minetest/minetest/issues/2094 -- Fix offset being ignored by inventory bar HUD by rubenwardy 15:54 sofar celeron55: effectively, my video posts are exactly that 15:54 sofar celeron55: of course I'm not terribly motivated to do them on stuff that others are contributing much, which is the downside of it 15:56 celeron55 and it's also a bit of a problem for me to do it because people expect me to lead things, not just follow whatever happens to be happening 15:56 celeron55 it would be much better for someone to do who isn't very involved in doing actual development but is interested in just writing 15:57 sofar nod. I hate writing myself but a dev blog of some sorts would maybe be nice 15:57 sofar alternatively, would you consider doing a twitter/g+/something else instead? 15:58 sofar some place where one account retweets/shares things that devs share? 15:58 sofar I use G+ for my tech stuff, but, I could care less 15:58 sofar it may be interesting to have a twitter account reshare stuff like my video dev explanations 15:59 sofar and/or notable commits and events 15:59 celeron55 well we have a twitter account which is basically dead 15:59 celeron55 some forum moderator type people have access to it, not sure who exactly 16:00 celeron55 this one https://twitter.com/MinetestCommune 16:00 rubenwardy I have access 16:00 rubenwardy If I can remember the password 16:00 sofar alternatively we can have this run by the #minetest-project folks 16:02 celeron55 this probably should be discussed on #minetest-project 16:03 celeron55 not sure where to start in there though 16:21 rubenwardy Rebased #2094 16:21 ShadowBot https://github.com/minetest/minetest/issues/2094 -- Fix offset being ignored by inventory bar HUD by rubenwardy 19:00 rubenwardy Rebased #2094 19:00 ShadowBot https://github.com/minetest/minetest/issues/2094 -- Fix offset being ignored by inventory bar HUD by rubenwardy 19:13 paramat unless there are objections i'll merge game#1019 (obvious bugfix) and game#1014 (trivial biome tweak) later. any mtgame dev want to test faster fire? game#1017 the faster burnout is useful < PilzAdam nore sfan5 19:13 ShadowBot https://github.com/minetest/minetest_game/issues/1019 -- Farming: Override dirt_with_dry_grass to enable cultivation by paramat 19:13 ShadowBot https://github.com/minetest/minetest_game/issues/1014 -- Default/mapgen: Denser rainforest, fewer logs by paramat 19:13 ShadowBot https://github.com/minetest/minetest_game/issues/1017 -- Fire: Faster spread and burnout by paramat 19:14 paramat #3959 needs updating then i hope to merge it later 19:14 ShadowBot https://github.com/minetest/minetest/issues/3959 -- Mapgen: Optimise cave noises and tunnel excavation by paramat 19:19 rubenwardy paramat, #2094 19:19 ShadowBot https://github.com/minetest/minetest/issues/2094 -- Fix offset being ignored by inventory bar HUD by rubenwardy 19:21 paramat thanks for rebasing 19:22 paramat added milestone 19:25 paramat needs review by someone more expert at hud than me 19:29 rubenwardy hmmmm was involved in hud 19:32 paramat this should be fixed, it has waited a year 19:45 sofar I've left a bunch of feedback (mostly positive) 19:46 sofar I need to restrain myself from writing on the sandstone walls PR 19:50 paramat ok. i'm ok with having no sandstone walls 19:55 rubenwardy Separated the issues: 3970 19:55 rubenwardy Separated the issues: #3970 19:55 ShadowBot https://github.com/minetest/minetest/issues/3970 -- Graphical Bugs when drawing inventory hud 20:09 paramat ok 22:41 Fixer logined to ESM, got crash with this: http://pastebin.com/raw/JvV6H36m 22:44 Fixer esm is nearly newest git 22:45 Fixer sadly, no more info