insta icon indicating copy to clipboard operation
insta copied to clipboard

Add `---` to the end of snapshots

Open max-sixty opened this issue 1 year ago • 6 comments

(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?)

max-sixty avatar Jun 15 '24 22:06 max-sixty

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.

mitsuhiko avatar Aug 01 '24 11:08 mitsuhiko

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...)

max-sixty avatar Aug 02 '24 06:08 max-sixty

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.

mitsuhiko avatar Aug 02 '24 09:08 mitsuhiko

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?

max-sixty avatar Aug 02 '24 17:08 max-sixty

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.

mitsuhiko avatar Aug 08 '24 21:08 mitsuhiko

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: https://github.com/mitsuhiko/insta/issues/457#issue-2169522883 — 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)

max-sixty avatar Aug 09 '24 04:08 max-sixty