On Tue, Apr 03, 2018 at 06:27:55PM +0200, Johannes Schindelin wrote:
> I am very, very grateful for the time Peff spent on reviewing the previous
> iteration, and hope that he realizes just how much the elegance of the
> event-stream-based version is due to his excellent review.
Unfortunately I ran out of time this week to give this version an
equally careful review, and I'm about to go on vacation for a few weeks.
I did give a cursory look over it, and the new maybe_remove_section() is
much more pleasant. So aside from a few minor nits I pointed out, this
generally looks good.
One thing I'd like to have seen is a few more tests covering exotic
cases that I turned up in my earlier review. Some of the weird multiline
cases I care less about, but we should probably cover at least:
1. Comment behavior when removing a section that isn't at the
beginning of the file.
2. Removing the final key from a section with a subsection.
Those should both be natural fallouts of the new method, but it would be
good to have test coverage.
Thanks for reworking this, and if it's still not merged when I get back,
I promise to review it more carefully then. :)
-Peff
