Closed ploeh closed 10 years ago
I'm getting a reasonably good sense on the direction here. About the errors (judging only from a visual inspection), it seems that only the first page will be filled to PageSize (all entries after the PageSize'th entry will generate a new page, AFAICT), and the actual writing sequence (update index, update previous, create next) is not the one we have discussed (create next, update previous, update index). But as you are playing the Devil's Advocate game, those findings (if correct) are not alarming ( yet :) ).
How would you like to proceed ? I have no problem with merging this as-is, to be able to follow along in small steps.
Agreed, none of your findings are alarming yet, because I already know about them :)
Still, it's good to call these out, so I've added them as sub-tasks on the story.
How to proceed is up to you. Personally, if I were you, I'd merge this PR, because it doesn't break any existing features, and that will make it easier for you to follow along.
As an alternative, I can stay on this appending-writer branch, and keep on adding new commits to it. That's no problem from my perspective, but might make your review tasks harder, because now you need to remember, how much of the branch you've already reviewed. That also mean that the 'folded' Files Changed GitHub view becomes less useful to you, because it includes the sum of all changes, including changes you've already seen. Instead, you'd have to rely more heavily on reviewing each commit separately.
This is very much a work in progress, but I thought that instead of sitting on a branch for weeks, it would be better to submit the PR now, in order to make it realistic to do a review of the code as it currently is.
There are lots of features missing, and too much duplication in both test and production code. There are also errors in the implementation. Some of these errors are deliberate, because I'm using the Devil's Advocate technique, but if you see any, please raise them.