On Mon, 2016-10-24 at 22:09 -0500, Hajime Fujita wrote: > On Oct 24, 2016, at 4:05 PM, Anton Lundin <glance at acc.umu.se> wrote: > > On 24 October, 2016 - Colin Leroy wrote: > > > On 24 October 2016 at 20h58, Tanu Kaskinen wrote: > > > > It would be great to have more people reviewing patches, but I don't > > > > know how to acquire those people. I don't think the reviews have to > > > > necessarily be done by someone with a title of "maintainer", but on > > > > the other hand, giving much trust to drive-by contributors seems risky > > > > too... > > > > > > Reviewing is hard when you don't have an extensive knowledge of the > > > codebase... I'd propose to do it, but I'd suck at it - "yeah, seems > > > fine to me" :D Reviewing is a good way to gain knowledge of the codebase, though. Even if the reviews weren't very thorough in the beginning, I'd still be willing to pay that price if the new reviewer was going to stay around for a longer time. (When you said "I'd propose to do it", you probably didn't mean becoming a long-term reviewer, but I thought I should make this point anyway.) > > I think we can file the raop2 code under a quite special category. It > > doesn't have a big inpact outside of the raop2 code, and those bits are > > quite trivial to review. I just read them my self and they looked ok to > > me =) Are there any patches in the set that you'd be comfortable if I tagged them with "Reviewed-by: Anton Lundin <glance at acc.umu.se>"? > That sounds interesting. Not sure if itâ??s a good idea to essentially > â??skipping" reviews for non-raop2 patches, but it makes a lot of sense > to focus on the patches that touch the core and other modules. > > Actually as an author of the patches my expectation for the review > was to make sure > a) we didnâ??t screw up when making changes to other parts of PA > b) naming conventions are acceptable > c) pulsecore API usages are correct > > Any other concerns from maintainerâ??s point of view? I think this is > also a good opportunity for me (and potentially others) to learn > about the review process. Well, I don't really have a checklist in my mind when doing reviews, but there's one point that gets easily forgotten by people who write patches (and sometimes I also forget to check it when reviewing): the commit message should clearly explain why the change is made, what problem it tries to solve. Regarding the code itself, I read the changes and try to understand why each individual change is done. If I can't understand, I'll ask for clarification. While figuring out how the code works, I may get ideas about how to do it better, or weird formatting or "bad" naming may irritate enough for me to complain. Also, I try all the time to think of ways how the code may break - maybe a pointer dereference isn't always safe, or maybe allocated resources aren't always freed properly etc. > > On the other hand, the raop2 code itself is quit the opposite. It > > requires understanding of raop2 and pa. > > > > Here comes the point: The current raop2 code is pretty much unusable. > > There are probably very few who have such old devices that they work > > with that code, and the changes only affects raop2 users. > > This is a very valid point. Iâ??d say this was already true three years > ago when I started working on this project. And it was so > disappointing to know that module-raop-sink didnâ??t work with most of > the raop devices in the market despite its name. Thatâ??s the why I > started working on this. > > > I'd love to see the code merged, and even with the > > module-raop-discover module disabled by default. That way this change > > only affects users who really wants and uses this feature, and probably > > need this code to get their gear working. > > FYI: in my understanding itâ??s already disabled by default. Yes, it is. > > So my suggestion is to take a look at the non-raop2 patches so the > > maintainers are happy with those, and merge the lot. Indeed, that may be a better alternative than keeping the patches waiting for review indefinitely. -- Tanu
