Determine when new comment lines are needed for itemized blocks by ytmimi · Pull...

Copy link

Collaborator

Nice! I think this is probably the right route to go, though out of an abundance of caution, could you add some additional test cases (above and beyond what you already have) which will actually hit the width boundaries and force a wrap? That should be doable either via some longer inputs (and/or with added indentation levels/nested items/etc.), or by setting comment_width to a sufficiently low value

ytmimi commented 20 days ago

could you add some additional test cases which will actually hit the width boundaries and force a wrap? That should be doable either via some longer inputs (and/or with added indentation levels/nested items/etc.), or by setting comment_width to a sufficiently low value

Sure thing. I'll include some more tests for this PR!

ytmimi commented 18 days ago

@calebcartwright I've gone ahead and added the additional test cases that force the comments to wrap. let me know if there's anything else you feel we should be testing to round out the PR.

One case where the proposed changes would be problematic would be when an itemized block opens with an empty comment line, e.g.

(note no trailing spaces after the first opener)

//
// - some itemized block 1
// - some itemized block 2

Haven't dug into the cause, but that input with the proposed changes would result in the first line getting stripped, and the opening space in the first item getting dropped, e.g.

//- some itemized block 1
// - some itemized block 2

So we'll need to iterate on this a bit more

ytmimi commented 14 days ago

I went back and took a look at this. There was a logical bug in buffer_contains_comment. Basically we weren't taking into account the scenario that you posed where the comment buffer already contained an empty comment. I fixed that issue and added some clarifying comments to that method. Also included your input in two new test cases!

Looks good, thanks for following up. My last ask (perhaps more of a question than a request) is whether it'd be worthwhile to include any of these tests but in the default mode (wrap_comments disabled)? I wouldn't anticipate any issues currently, but could see it potentially being useful in the future as a defensive type of test should this part of the codebase be modified

ytmimi commented 13 days ago •

Added warp_comments=false version of all the tests. I agree that these could be helpful in the future to make sure that we maintain the same behavior