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

Inline snapshots: do not indent empty lines. #8277

Merged

Conversation

scotthovestadt
Copy link
Contributor

@scotthovestadt scotthovestadt commented Apr 5, 2019

Summary

Problem as reported by @zertosh :

One issue I found though is that if your output has new lines somewhere, they become just indented whitespace - as expected. But most editors have automatic trailing whitespace removal, so you end up changing the test just by saving.

The obvious concern here is that, if for whatever reason a snapshot is relying on whitespace on a certain line, we don't want to impact that... even if some editors are going to automatically remove it.

To resolve, I've just stopped indenting empty lines IF they are completely empty.

Test plan

  • Added unit test to verify behavior. The test previously fails, it now passes.
  • Added e2e tests for behavior.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@scotthovestadt scotthovestadt merged commit 01044da into jestjs:master Apr 5, 2019
@zertosh
Copy link
Contributor

zertosh commented Apr 6, 2019

Any chance this can go out in a patch release?

@SimenB
Copy link
Member

SimenB commented Apr 6, 2019

Seems like a bug in those editors - removing trailing whitespace in a template literal changes the string. In this case (snapshots), it doesn't really matter or course :)

@scotthovestadt
Copy link
Contributor Author

@zertosh Will push out soon.

@SimenB Yeah, I agree it's a little strange to have editors changing things inside of template literals, but I'm able to reproduce it

@alfaproject
Copy link

Also, git does that as well if you configure it to do so, which I usually do

@zertosh
Copy link
Contributor

zertosh commented Apr 28, 2019

Can I bug someone to please do a release with this fix? :)

@scotthovestadt
Copy link
Contributor Author

@zertosh Yeah, plan to do shortly... but FYI at Facebook we're now on master branch so you should have it there :)

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants