Help us review and merge PRs

For people working on the C++ code.
User avatar
rubenwardy
Moderator
 
Posts: 5503
Joined: Tue Jun 12, 2012 18:11
Location: United Kingdom
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Help us review and merge PRs

by rubenwardy » Sun Mar 25, 2018 23:49

Reviewing PRs takes rather a long time to do properly, and devs only have a certain amount of time to contribute to engine work.

Help us merge the PRs you like by, in order of difficulty:

  1. Showing your approval using reactions on the first post of the Github issue
  2. Post constructive comments and try to convince other people of its usefulness if needed.
    Don't just do "+1" or "looks good!" as that's annoying and not constructive.
  3. Post PRs in the #minetest-dev IRC channel to ask devs to review them (don't ping anyone though as that's rude!)
  4. Test to find bugs. Do it with a high attention of detail, and say how you tested it.
    We'll trust your testing more if you say what you've done and if you consistently find issues in things you test
  5. Review the PR to find issues. Anyone can use the review feature to point out issues they find.
    Pointing out issues before a dev has to means we can use our reviewing time more productively.
    Don't just approve a PR as we'll just assume you're doing it because you like the concept.
    Use the emoji reactions to show your approval. Only use the review feature if you've checked the code.
    Make sure to say whether you've tested it (and how) and also how detailed your code review was.
  6. Kidnap a dev (note: may be illegal).

Number 4 and 5 are critical. I very rarely see testing or reviews by non-devs.
Before I was a developer, I reviewed PRs for easily two years.

In the mean time, we've already made some progress with Lua Unit Tests and code clean ups which should help with reviewing PRs and avoiding regressions. Hopefully we'll eventually get to a state where a large number of important issues are caught by testing, although this will be difficult

Interested in how Minetest is developed? Have a look at the Developer Road Maps and Worklists, the To do list on the dev wiki, and consider watching the engine repository.
 

twoelk
Member
 
Posts: 1253
Joined: Fri Apr 19, 2013 16:19
Location: northern Germany
GitHub: twoelk
IRC: twoelk
In-game: twoelk

Re: Help us review and merge PRs

by twoelk » Mon Mar 26, 2018 13:41

hmm, maybe "adopt a dev" as more legal option for #6 ? - could link to a list of patreon pages ;-P

and there is always the option to offer a bunch of bananas to keep the random gang of code-monkeys focused - or at least somewhat interested in something the "users" might consider more important than perfect code.

I have always wondered wether good game and beautiful code exclude each other.

btw - I still think a slot for a temporary public dev-testing server might be useful. something like a server that is only started for certain tests - maybe something like an event on semiregular basis with the stats published and discussed with non dev players. might be a good place to get more people interested in the dev side of minetest.
 

User avatar
rubenwardy
Moderator
 
Posts: 5503
Joined: Tue Jun 12, 2012 18:11
Location: United Kingdom
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy

Re: Help us review and merge PRs

by rubenwardy » Wed Mar 28, 2018 21:38

twoelk wrote:hmm, maybe "adopt a dev" as more legal option for #6 ? - could link to a list of patreon pages ;-P.


Donating may work well at encouraging them to invest more time in, especially if that time would otherwise be spent doing extra shifts. Bounties are particularly good at this - not only do you get to work on the project, you get paid for that particular feature! That being said, donations are unlikely to make someone able to give up their day job so ultimately they'll still have a maximum amount of time available to give to the project. If donations were enough to pay someone full time, that is a whole new can of worms.

That's not the point of this topic anyway, the point of this topic is to try and encourage more people to get involved in development.

I only know of two devs that have public donation pages (and one is me):

 

User avatar
Hybrid Dog
Member
 
Posts: 2671
Joined: Thu Nov 01, 2012 12:46

Re: Help us review and merge PRs

by Hybrid Dog » Mon Apr 02, 2018 19:44

twoelk wrote:I still think a slot for a temporary public dev-testing server might be useful. something like a server that is only started for certain tests - maybe something like an event on semiregular basis with the stats published and discussed with non dev players. might be a good place to get more people interested in the dev side of minetest.

Somebody could create a minetest server with promising PRs merged. To lure players in, WIP mods and mods which aren't used on productive servers (such as an automatic rocket launcher) could be added (and removed if they caused too much lag).

‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪‮
‮‪
 

User avatar
Wuzzy
Member
 
Posts: 3234
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy

Re: Help us review and merge PRs

by Wuzzy » Sat Oct 13, 2018 21:15

Probably one of the most frustrating problems for contributors to run into when it comes to PRs is “rebase needed”. I don't want to know how many fully-coded fully-tested PRs have failed because of a “rebase needed”.

Anyway. Rubenwardy, are there special labels for which community testing is especially desired (link in first post plz :-))?
My creations. I gladly take any bitcoins you have lying around: 17fsUywHxeMHKG41UFfu34F1rAxZcrVoqH
 


Return to Partly official engine development



Who is online

Users browsing this forum: No registered users and 1 guest