Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

varlanv
Copy link

@varlanv varlanv commented May 30, 2025

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
Copy link
Author

varlanv commented May 31, 2025

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
Copy link
Member

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
Copy link
Author

varlanv commented May 31, 2025

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 {
...
}

… //selfieonce

Key changes:
- Implement escaping rules for escaping multiline strings, block-style comments /**/, javadoc-style comments /***/ during //selfieonce replacement
- Add comprehensive test suite in `RemoveSelfieOnceCommentTest` to verify correct behavior in various scenarios
- Update one test case in `InteractiveTest`
@varlanv varlanv force-pushed the fix/81-selfieonce-rewrite-escaping branch from e6ded20 to c038b41 Compare May 31, 2025 18:22
@varlanv
Copy link
Author

varlanv commented May 31, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants