[pulseaudio-discuss] Revisiting raop2 patches

[Tanu Kaskinen]

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