crankyoldgit / IRremoteESP8266

Infrared remote library for ESP8266/ESP32: send and receive infrared signals with multiple protocols. Based on: https://github.com/shirriff/Arduino-IRremote/
GNU Lesser General Public License v2.1
2.98k stars 832 forks source link

Looking for people to do code reviews. #115

Closed crankyoldgit closed 7 years ago

crankyoldgit commented 7 years ago

Hi, Do you know how to program? Are familiar with C/C++ or other languages (python/go/etc)? If so, you may be able to help us.

Under the principle that many & different eyes might catch mistakes that a code author might miss, I'm after people to review all of the code prior to commits. Ideally, we'd have more than one person review a commit, but starting with at least one is a first step.

What the job of a code reviewer entails is someone to check if the code looks sane, would be similar to how they would implement it, check for any obvious boo-boos, offer advice, and offer constructive criticism. A code review(er) is not responsible for making sure there are no bugs, or anything. They are never to blame if something goes wrong. All responsibility is with the code author basically. Not you. So there is little pressure.

Currently @roidayan (mostly) & @markszabo are doing a lot of the reviews (and myself vice-versa) and I feel bad hitting them up for every change I make. So, it'd be real nice to them to if you'd like to sign up so we can spread the workload around.

If you feel like you can do this, drop a comment on this issue indicating that, or if you have further questions about the task.

Thanks.

dbranagh commented 7 years ago

Hi, Would effort be better spent merging this back into http://z3t0.github.io/Arduino-IRremote/ as another supported board variant? Then there is one codebase. (I am not aware of all the changes that had to be made to get this to work on ESP8266, and personally I use it as a library to a variant of this project: https://github.com/enc-X/mqtt-ir-transceiver/ ) ..just a thought.. Thanks for the code to date!

crankyoldgit commented 7 years ago

In an ideal world, yes. However, there are a lot of subtle differences between the this platform and the Arduino/ATMegas etc. It would be a huge effort to merge to the two at this stage.

rahuljawale commented 7 years ago

How does one sign-up for code reviews? I have software development background but I am new to Arduino/ESP development, can I still contribute?

crankyoldgit commented 7 years ago

You have basically just signed up. ;-) Yes, of course you can contribute. All constructive input is welcome. e.g. Bug reports, Pull requests, reviews etc etc. I'll add you to some Pull Requests (PRs) now.

crankyoldgit commented 7 years ago

So, it turns out I can't add arbitrary people to do official code-reviews in the github gui. So, in the meantime I'll just use @ references until I find a better way.

rahuljawale commented 7 years ago

That's great! Thanks. I can see couple of Pull requests assigned to me. Will take a look.

markszabo commented 7 years ago

@crankyoldgit I can add rahuljawale (and later other code reviewers) as collaborators, but then they will have the same access as you (full push access). There is sadly no other role in Github as far as I know. We can wait for 3-5 review from each of the code reviewers, and if they are doing it good, we can add them as collaborators. Or we can also add them right away, since in the worst case we will have to revert some commits, if they mess up something. What do you think?

crankyoldgit commented 7 years ago

@markszabo Yeah, github's built-in code-review mechanism is a little limited. :-/ I've had a quick look at a couple of other bolt-on tools to see if we can add reviewers without them having commit privs. Either we find a tool, or as you suggest, we can add them as collabs after a few reviews.

I'm a little hesitant to grant write access willy-nilly but lets try this way for a bit and see how we go.

crankyoldgit commented 7 years ago

@marcmerlin Can I tempt you to assist in this capacity/role?

marcmerlin commented 7 years ago

@crankyoldgit haha, nice try :) I've been suckered too many times into doing this for ESP32, and I'm not done there yet, that it's likely not resonable that I sign up for yet another lib :) Also, I'll freely admit that I'm not super keen on spending too much effort on ESP8266, it's not a fun chip to work on (especially after working on ESP32)

crankyoldgit commented 7 years ago

@markszabo I think @rahuljawale has served the probation period satisfactorily. :-) Can you please add him as a contributor? Thanks in advance.

crankyoldgit commented 7 years ago

Ping @markszabo

markszabo commented 7 years ago

Sorry, I missed this first. Added now. Thank you for all your work :+1:

crankyoldgit commented 7 years ago

@scop As you've contributed, and know c++ well enough. Care to assist in this role?

scop commented 7 years ago

Maybe some day, but for now I have quite a bit on my plate already.

rahuljawale commented 7 years ago

Sorry folks I have been away due to work commitments. I have a long weekend coming up here in India, I can take up a few reviews if are still pending.

crankyoldgit commented 7 years ago

@rahuljawale No worries. Real jobs come first. :) I'll add you to some of the open PR/reviews.