These cause too much nagging that make comments about correctness go lost in the noise.
Remove file and class order guidelines #5201
pull sipa wants to merge 1 commits into bitcoin:master from sipa:order changing 1 files +0 −14-
sipa commented at 9:01 AM on November 3, 2014: member
-
Remove file and class order guidelines be0e0703f2
-
laanwj commented at 9:04 AM on November 3, 2014: member
Ah yes, I was intending to do this for a while. ACK.
-
laanwj commented at 12:53 PM on November 3, 2014: member
I think even with this we should split up the review of difficult pulls into two phases:
- In the first phase, we allow only only commentary on the correctness of the code and the overall concept
- Then just before merging we can go into a "clean up" phase, and nit about extra spaces, indentation, comment style and other minor details
- laanwj added the label Docs and Output on Nov 3, 2014
-
jgarzik commented at 2:11 PM on November 3, 2014: contributor
ACK
-
Diapolo commented at 2:43 PM on November 3, 2014: none
Fell free to do that, I just think it's anoying to make such nits for core devs anyway. If all would use this convention, it wouldn't even cause the need for a "cleanup phase" nor the need for such a pull ;).
It's also likely that not many will ever contribute to such a cleanup phase... if the intention is: "Please Dia don't always "spam" our pulls with nits!" I'm fine with that, but will rethink my motivation to improve code quality (style wise ^^) for this project.
-
gavinandresen commented at 2:52 PM on November 3, 2014: contributor
ACK.
Please Dia don't always spam our pulls with nits.
- gavinandresen referenced this in commit c969096070 on Nov 3, 2014
- gavinandresen merged this on Nov 3, 2014
- gavinandresen closed this on Nov 3, 2014
-
laanwj commented at 3:15 PM on November 3, 2014: member
@Diapolo Yes, if everyone would keep exactly to the convention. But we're human, not robots (ok, speaking just for me). While thinking deeply about how code and data structures should work, reordering and writing new code, it's easy to make a stylistic "mistake" here and there. Focusing too much on them is distracting. This is true during review as well.
I'm not sure how to handle this better - the two-phase approach is just a proposal that may not actually work either - but 'Please Dia don't always spam our pulls with nits' sounds good to me for now.
-
jgarzik commented at 4:33 PM on November 3, 2014: contributor
Indeed. Referencing the Linux kernel, there were periodic cleanup patches from "janitors" who are typically just entering the codebase / learning the code. This is better than nagging on every PR.
-
sipa commented at 4:46 PM on November 3, 2014: member
At Google there were very strict rules about ordering of includes etc, and this worked well. But that was with automatic tools for reordering, and their code-review tool would tell you about this immediately (and many more things), so it was much easier to make sure that everybody always knows and used the same system.
Here we tried, and I don't think the benefits are worth the effort.
- MarcoFalke locked this on Sep 8, 2021
