#81: Implement ignoring of multiline strings and comments for //selfieonce

[varlanv]

Key changes:

• Implement escaping rules for escaping multiline strings, block-style comments /**/, javadoc-style comments /***/ during //selfieonce replacement
• Add comprehensive test suite in RemoveSelfieOnceCommentTest
• Update one test case in InteractiveTest

This implementation can actually work more efficiently by working entirely on CharSequence to avoid generating strings back and forth from com.diffplug.selfie.guts.Slice. But if I change receive/return types to CharSequence, it won't connect with Slice class, because it expects to receive String.

Please don't freak out by huge diff, most of it are test cases to ensure there is no regression, and to illustrate what cases new functionality covers.

[varlanv]

Here are some examples of how new implementation will handle //selfieonce replace.

These will be replaced:

• //selfieonce

• // selfieonce

• //selfieonce (with trailing whitespaces)

• If //selfieonce comment is sitting alone on the line, the line will be trimmed until first \r or \n symbol to the right and to the left of the comment

• If //selfieonce comment is placed next to some source code, then everything to the left of // will be trimmed until first non-whitespace character, and everything to the right will be trimmed until first \n or \r.

These would be replaced before, but will not be replaced with PR version:

• "//selfieonce"
• """ //selfieonce """
• /* //selfieonce */
• /** //selfieonce */
• //selfieonce//selfieonce

"""
//selfieonce
"""

/*
//selfieonce
*/

[nedtwigg]

Wonderful, thanks! Out of curiosity, did you bump into a case where you had selfieonce in a test string? I did while making the test suite lol, very confusing, but I figured it would hopefully be rare for endusers. Very happy to have it fixed for real, but I'm curious about the circumstances that exposed the bug to you :)

[varlanv]

Hi @nedtwigg, thanks for the interest in the PR.
I didn't actually bump into this problem myself. I just think this library introduces interesting approach for snapshot testing that I've never seen before, so I wanted to check how it is implemented underneath, and stumbled upon this bug ticket.

I think //selfieonce in string literals is indeed extremely rare scenario. Special handling for it is there just for the completeness.

Probably more likely end users might want to put it in the comments like so:

/**
* This test suite heavily relies on selfie library
* for snapshot testing. For updating snapshots,
* put //selfieonce comment anywhere in the file and re-run test suite.
*/
class SomeTest {
...
}

[varlanv]

I saw that Ci was failing with spotless check, so I ran spotlessApply and force-pushed changes, hopefully it will be okay this time.

[nedtwigg]

Sorry this has sat for so long. To close the loop:

• this is excellent code, and it fixes a cornercase that Selfie currently has
• but it is quite a lot of code, and I'm not sure it's worth the complexity
• there are external systems that might want to replicate Selfie's parsing (such as [jvm] Using selfie with Gradle Develocity Test Distribution #553) and it's easier to do that if Selfie is doing something simple

I plan to not merge this code but leave the PR open. I think Selfie will keep it's comment detection / fixing to be just the naive super simple logic it has now:

selfie/jvm/selfie-lib/src/commonMain/kotlin/com/diffplug/selfie/guts/CommentTracker.kt

Lines 60 to 61 in 319a1b9

	for (comment in listOf("//selfieonce", "// selfieonce", "//SELFIEWRITE", "// SELFIEWRITE")) {
	val index = content.indexOf(comment)

But if people bump into problems with this, I hope they share their experience and I am open to reconsidering.