Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR review #129

Closed
li-s opened this issue Aug 31, 2020 · 5 comments
Closed

PR review #129

li-s opened this issue Aug 31, 2020 · 5 comments

Comments

@li-s
Copy link

li-s commented Aug 31, 2020

Hello guys, I have a quick question.

If reviewing PR, and the code has multiples of the same mistake (eg. wrong indent for multiple files), do we flag every single mistake, or just flag one? I'm thinking that if you flag every mistake, it will just make the other member angry at why you are focusing on such minor mistakes.

Any thoughts on this?

@damithc
Copy link
Contributor

damithc commented Aug 31, 2020

Good observation @li-s . The points in the following link sort of touches on that situation. Have a look and post here if you have further doubts.
image

@li-s
Copy link
Author

li-s commented Aug 31, 2020

So for each coding violation that is the same, I should just write "I noticed this issue as well"? It seems like it will clog the comments section.

@peter-yeh
Copy link

Hi @damithc , I have this issue as well, when the repo I am reviewing also commits many of the same mistake. After reading the Best practices for reviewing PRs, I still did not find the answer I am looking for.

@damithc
Copy link
Contributor

damithc commented Aug 31, 2020

So for each coding violation that is the same, I should just write "I noticed this issue as well"? It seems like it will clog the comments section.

Hmm... that's not what the guide recommends, right?
image

For example, adding this comment in one place is enough. Let the author finds the other places by him/herself.

This line seems to exceed the recommended max line length. I noticed the same problem in a few other places too.

@li-s li-s closed this as completed Aug 31, 2020
@li-s
Copy link
Author

li-s commented Aug 31, 2020

Ok thanks, so I just wite that line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants