Hello everyone, I have completed introducing some code from v-hasu's branch into mine, mainly for Features, FeatureVector and FeatureManager classes. I have pushed the changes to https://github.com/ayshtmr/xapian/tree/letor-update. I am now proceeding to write unit tests for feature modules. There are a few things that I wanted to clarify: 1. I have introduced a lot of code in a single commit ( https://github.com/ayshtmr/xapian/commit/2abaf6b8d64b1624699c3f490b6dc441e1c6cdf3 ) Is it fine or should I try to split things up? 2. There is a lot of commented code in my branch right now that needs to be removed. Is it wise to take care of it with each commit I push or should it be done later, say, when some stability is reached (post mid-term?) Regards, Ayush -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xapian.org/pipermail/xapian-devel/attachments/20160606/fb2e406e/attachment.html>
On Mon, Jun 06, 2016 at 01:13:14PM +0530, Ayush Tomar wrote:> I have completed introducing some code from v-hasu's branch into mine, > mainly for Features, FeatureVector and FeatureManager classes. I have > pushed the changes to https://github.com/ayshtmr/xapian/tree/letor-update. I > am now proceeding to write unit tests for feature modules.Great!> There are a few things that I wanted to clarify: > > 1. I have introduced a lot of code in a single commit ( > https://github.com/ayshtmr/xapian/commit/2abaf6b8d64b1624699c3f490b6dc441e1c6cdf3 > ) > Is it fine or should I try to split things up?A good rule of thumb is to aim for a maximum of 800 line changes. (This is based on observations of how long it takes to review code, and how long people can be effective at reviewing code.) Obviously for complex things, you want to aim somewhat lower. I think in this case we'll want to split the commit up in order to review it, and then squash it all down again when we merge. (That happens sometimes.) For instance, there's a rename of getlabel -> get_label which you could pull out into one commit (which is then a fairly straightforward commit to check, since providing the end result of the PR compiles it will have caught all the cases). Probably the easiest way of doing that is to do something like the following: * interactive rebase and mark that commit for edit * when you're dropped into the shell, use git reset --soft HEAD^ to get the commit into the stage * use git reset -p to remove everything but the getlabel rename lines from the index * git commit (the rename changes) * git add everything else back in and git commit that (or, more likely, continue to split into further commits) * git rebase --continue when you're done I think if you look carefully at that commit you'll also discover that the letor_features.cc changes contain a lot that's unwanted whitespace changes and should be dropped from the commit. (-b to git show and git diff can be helpful here, as it ignores whitespace changes. If it isn't there but shows up as an option when you're doing git add -p then it's a sign something's up, probably with your editor configuration.)> 2. There is a lot of commented code in my branch right now that > needs to be removed. Is it wise to take care of it with each commit > I push or should it be done later, say, when some stability is > reached (post mid-term?)I'd do it sooner rather than later, because it'll make the commits more manageable and also the files smaller, which is easier for you to deal with. What I'd do is to create fixup commits that remove the commented code, tied to whichever commits you want to get that removal into. Then you can squash them before we merge. (That's considerably easier for you, because you can focus first on getting the commit splits right, and then tidy up each one.) J -- James Aylett, occasional trouble-maker xapian.org
Hello James, I am really sorry but I just realised that I introduced code from v-hasu's master branch rather than gsoc2014. Will try to make the fixes asap. Thanks for your suggestions, though. I will take care of them from the beginning this time. Will open a PR once I am done with everything. Regards, Ayush On Mon, Jun 6, 2016 at 3:09 PM, James Aylett <james-xapian at tartarus.org> wrote:> On Mon, Jun 06, 2016 at 01:13:14PM +0530, Ayush Tomar wrote: > > > I have completed introducing some code from v-hasu's branch into mine, > > mainly for Features, FeatureVector and FeatureManager classes. I have > > pushed the changes to > https://github.com/ayshtmr/xapian/tree/letor-update. I > > am now proceeding to write unit tests for feature modules. > > Great! > > > There are a few things that I wanted to clarify: > > > > 1. I have introduced a lot of code in a single commit ( > > > https://github.com/ayshtmr/xapian/commit/2abaf6b8d64b1624699c3f490b6dc441e1c6cdf3 > > ) > > Is it fine or should I try to split things up? > > A good rule of thumb is to aim for a maximum of 800 line > changes. (This is based on observations of how long it takes to review > code, and how long people can be effective at reviewing code.) > Obviously for complex things, you want to aim somewhat lower. > > I think in this case we'll want to split the commit up in order to > review it, and then squash it all down again when we merge. (That > happens sometimes.) > > For instance, there's a rename of getlabel -> get_label which you > could pull out into one commit (which is then a fairly straightforward > commit to check, since providing the end result of the PR compiles it > will have caught all the cases). Probably the easiest way of doing > that is to do something like the following: > > * interactive rebase and mark that commit for edit > * when you're dropped into the shell, use git reset --soft HEAD^ to > get the commit into the stage > * use git reset -p to remove everything but the getlabel rename lines > from the index > * git commit (the rename changes) > * git add everything else back in and git commit that (or, more > likely, continue to split into further commits) > * git rebase --continue when you're done > > I think if you look carefully at that commit you'll also discover that > the letor_features.cc changes contain a lot that's unwanted whitespace > changes and should be dropped from the commit. (-b to git show and git > diff can be helpful here, as it ignores whitespace changes. If it > isn't there but shows up as an option when you're doing git add -p > then it's a sign something's up, probably with your editor > configuration.) > > > 2. There is a lot of commented code in my branch right now that > > needs to be removed. Is it wise to take care of it with each commit > > I push or should it be done later, say, when some stability is > > reached (post mid-term?) > > I'd do it sooner rather than later, because it'll make the commits > more manageable and also the files smaller, which is easier for you to > deal with. > > What I'd do is to create fixup commits that remove the commented code, > tied to whichever commits you want to get that removal into. Then you > can squash them before we merge. (That's considerably easier for you, > because you can focus first on getting the commit splits right, and > then tidy up each one.) > > J > > -- > James Aylett, occasional trouble-maker > xapian.org > >-- Ayush Tomar Senior Undergraduate Dept. of Computer Engg. Delhi Technological University *(formerly Delhi College of Engineering)* -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.xapian.org/pipermail/xapian-devel/attachments/20160607/d488edb4/attachment.html>