Picking at minor things in PRs can be a big turn-off. I feel that folks should just edit the PR to make minor changes. That feels is far more collaborative and constructive than putting in comments about typos or suggestions to rename things.
That reminds me that one of the first contributions of a feature I did the BDFL thanked me then rewrote the feature their way and pushed it out as a release. I felt that I had been listened to and I could immediately upgrade to get the feature. That was a better outcome all around than being ignored or asked to rewrite if another way.
I'm a web sysop and support engineer. My skills are mainly in back-end: Java, Linux, Python, PostgreSQL, Git, and GitLab. Currently I'm learning front-end skills: JavaScript, and Ruby.
I feel it's better to make a computer do code linting, than to do this in a PR review. Nobody minds when a computer picks nits, but when people do, it's taken personally 😉
The review should focus on the logic, that the PR makes sense, and then run it though a linter for code style.
Bonus points to the reviewer for lint instructions or offer an editor.conf for the project.
In an ideal world, that is a great way to go about it. But most maintainers may not find the time to do all that. A short review in text is much faster, if more frustrating for the person opening the PR.
Picking at minor things in PRs can be a big turn-off. I feel that folks should just edit the PR to make minor changes. That feels is far more collaborative and constructive than putting in comments about typos or suggestions to rename things.
That reminds me that one of the first contributions of a feature I did the BDFL thanked me then rewrote the feature their way and pushed it out as a release. I felt that I had been listened to and I could immediately upgrade to get the feature. That was a better outcome all around than being ignored or asked to rewrite if another way.
I feel it's better to make a computer do code linting, than to do this in a PR review. Nobody minds when a computer picks nits, but when people do, it's taken personally 😉
The review should focus on the logic, that the PR makes sense, and then run it though a linter for code style.
Bonus points to the reviewer for lint instructions or offer an editor.conf for the project.
In an ideal world, that is a great way to go about it. But most maintainers may not find the time to do all that. A short review in text is much faster, if more frustrating for the person opening the PR.
But, surely it is better for you to learn the style so that your next PR doesn't need to be rewritten.
In general we are trying to build a community not just add your feature.