Help us review and merge PRs

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

Help us review and merge PRs

by rubenwardy » Post

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. Bumping 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

    Also, look out for this label: Image
  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.
Renewed Tab (my browser add-on) | Donate | Mods | Minetest Modding Book

Hello profile reader

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

Re: Help us review and merge PRs

by twoelk » Post

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: 6972
Joined: Tue Jun 12, 2012 18:11
GitHub: rubenwardy
IRC: rubenwardy
In-game: rubenwardy
Location: Bristol, United Kingdom
Contact:

Re: Help us review and merge PRs

by rubenwardy » Post

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.
Renewed Tab (my browser add-on) | Donate | Mods | Minetest Modding Book

Hello profile reader

User avatar
Hybrid Dog
Member
Posts: 2828
Joined: Thu Nov 01, 2012 12:46
GitHub: HybridDog

Re: Help us review and merge PRs

by Hybrid Dog » Post

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: 4786
Joined: Mon Sep 24, 2012 15:01
GitHub: Wuzzy2
IRC: Wuzzy
In-game: Wuzzy
Contact:

Re: Help us review and merge PRs

by Wuzzy » Post

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 :-))?

Post Reply

Who is online

Users browsing this forum: No registered users and 1 guest