Lots of CI failures today for various reasons, slightly behind on PR review, could do that during today’s meeting.
Want to try your hand at PR reviews? Lots of changes from JHorton and I’m kinda slow to respond. Eg this.
My major process for reviewing PRs is:
(before looking at code) Read over the goals of the PR and try to understand them, and decide whether it’s a good thing at all.
Assume Josh H adding stuff is a good idea
If it’s a good thing, then see whether the good thing actually got implemented by looking at the tests and ensuring that the good thing actually happens.
Ensure the tests also cover common failures (if relevant, maybe less is needed in an already-developed codebase with input validation and stuff)
If the above are all good, assume you’ll approve the PR unless the implementation is terrible. Ensure implementation is kinda documented and readable, but it doesn’t need to be a work of art or anything.