Skip to content
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

Add --- to the end of snapshots #506

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

max-sixty
Copy link
Collaborator

@max-sixty max-sixty commented Jun 15, 2024

(requires some thought / discussion)

This would allow us to handle ending newlines.

We'd need to do some work

  • to check newlines
  • to handle the transition when some files won't have these ending items, to only check those that do

(...possibly work we should do before we merge this?)

(requires some thought / discussion)

This would allow us te handle ending newlines.

We'd need to do some work
- to check newlines
- to handle the transition when some files won't have these ending items, to only check those that do

(...possibly work we should do before we merge this?)
@max-sixty max-sixty mentioned this pull request Jun 17, 2024
@max-sixty max-sixty requested a review from mitsuhiko July 7, 2024 19:18
@mitsuhiko
Copy link
Owner

We have the same problem with leading whitespace too. I think it needs addressing but I'm not convinced that this is the right way to do it.

@max-sixty
Copy link
Collaborator Author

I think it needs addressing but I'm not convinced that this is the right way to do it.

No prob ofc

We have the same problem with leading whitespace too.

Right — just tbc, if we have delimiters at the start & end, then it becomes possible to check for the number of newlines. IIUC, we previously gave up and just .trim everything because we didn't have a delimiter at the end.

(Note that this PR doesn't check newlines, it just adds the delimiters so that we can later check them...)

@mitsuhiko
Copy link
Owner

I'm thinking the right solution here would be that if

  • more than one trailing newline exist
  • or other trailing spaces
  • or any leading spaces

We add a unicode character to mark the beginning or end of it. I was thinking of inserting at the end and at the beginning or something similar. The diffing tool already also adds special unicode characters to indicate some significant whitespace to clarify what's going on.

@max-sixty
Copy link
Collaborator Author

more than one trailing newline exist

How about discriminating between zero and one trailing newline?


To ensure we're on the same page — at least for file snapshots — I think this is only a problem because of tools which change ending newlines (for example lots of repos have lints which enforce exactly one ending newline). Otherwise, we can just put the value after the header and we're done. Or am I missing something?

If that's the case, why do we need to handle the beginning?

@mitsuhiko
Copy link
Owner

So to be specific on the challenge: the leading whitespace is a problem in inline cases because of whitespace removal for indentation handling. If the leading whitespace is meaningful you lose the meaning in the assertions. The trailing one has similar challenges, but the newline specific case is indeed just for file snapshots due to editor settings.

@max-sixty
Copy link
Collaborator Author

the leading whitespace is a problem in inline cases because of whitespace removal for indentation handling

I would claim newlines in inline snapshots are a very tractable problem: #457 (comment) — inline snapshots are either short and contain zero newlines, or long and always have exactly one newline trimmed at the start & end...

(I agree indentation isn't possible to control like this, though also that's a less frequent issue)

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