On Tue, Dec 08, 2015 at 09:20:52AM +0100, Lukas Slebodnik wrote: > On (07/12/15 15:51), Petr Cech wrote: > >Hi Lukas, > > > >thank you for review. I will send new version of tests, in one patch. And I > >will address your comments. > > > >However I would like to shed light on why I made the changes gradually. API > >consists of three functions. I wanted every patch adding just one feature of > >the API. First, I tested the function to open a database. Then I tested the > >entry into the database, which influenced the opening. Finally, I tested > >reading. I tried to build in the spirit of building a mathematical theory. It > >helped me to think about different cases. And I thought it would help to > >reviewer too... > > > Yes, but it's difficult to review code is you rename and move functions. > It is inevitable in some cases. But not in this case. > The purpose of ticket is add new test cases and not to move and rename > change functions. It just only mean that function was not reusable in > the 1st patch and should be changed there and not in following patch. > > >Well, I remake it in a single patch. > > > Moreover, I am not a single person who means that splitting tests > to separate patches is best idea.
(I haven't read these patches at all, just a generic comment) I think even better than separating patches from functionality is separate the hunks that move something around (rename, split, ..) in tests from adding new test code. Then the reviwer can focus on making sure the first patches keep the same functionality and the second batch increases coverage. Of course there is no hard rule.. > > Nikolai had similar request in another review. > https://lists.fedorahosted.org/archives/list/sssd-devel%40lists.fedorahosted.org/message/DDA27YPA6B7ZCVPACJFPBT2RAZZWGJKL/ > > If you just add code then it's not a problem to review big patch > because you can read it as long article. > > LS > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
