On Sun, Apr 1, 2012 at 11:35 PM, Joachim Durchholz <[email protected]> wrote: >>> You'll have to explain what "atomic" is. This can range from five-liners >>> to >>> five-thousand-liners. >> >> >> That's my point exactly. > > > My point here is that a five-LoC commit, even if it's atomic, isn't worth > it. > And a 5000-LoC commit, no matter how well it fits the "atomic" definition, > is not reviewable and should be redesigned so that smaller atomic commits > can be made.
If it's a five-line orthoganl bugfix, this should probably be submitted as a separate pull request. I agree that such a small change within the project is probably too small for a single pull request (that's only useful if you don't plan on doing any additional work, which won't be the case for GSoC projects). > > A figure of a few hundred lines is not technically correct, but it will > nudge people in the right direction. (Which is good enough in this context.) > > On a related point: To use atomicity as a criterion, we need a definition. > My definition would be: "Cannot be broken into pieces without losing > property XYZ." However, I'm coming up blank with what XYZ is, and without > that, the criterion cannot be applied. In fact I think the definition of XYZ > is the salient point here. I was thinking that XYZ would be something along the lines of a passing test suite for the function. That may sound silly, but this is exactly when the code works, which means that it can be submitted. If the tests don't pass (assuming you've written all test cases already), then you still need to work on the code. And as I said, this will encourage good coding practices like writing tests before code, writing tests for internal functions (instead of just for the public interface), and writing documentation with the code. By the way, another idea we could try is to use a time based metric instead of a size one. So, for example, we could say that once a week (say), the student should submit what he has done (that works). I think reviewing a week's worth of code is reasonable. This should be more accurate than line- or commit-based metrics, I think, because the time to review code should be roughly proportional to the time it took to write it. Anyway, let's play with it. I think we should just see what works for each mentor/student pair. Aaron Meurer > > In general, I think both the definition of atomicity and ballpark figures > would be useful to newcomers (experienced developers already know what a > good pull request is). So something like this might work: > "Pull requests need to be 'atomic', i.e. [definition of 'atomic' goes here]. > If unsure, aim for a pull request size of around 300-500 lines of new code." > > > -- > You received this message because you are subscribed to the Google Groups > "sympy" group. > To post to this group, send email to [email protected]. > To unsubscribe from this group, send email to > [email protected]. > For more options, visit this group at > http://groups.google.com/group/sympy?hl=en. > -- You received this message because you are subscribed to the Google Groups "sympy" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/sympy?hl=en.
