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

Re-add trailing spaces to make prettier merge cleaner #12222

Closed
wants to merge 1 commit into from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Sep 27, 2024

Description

In #12206 it seems some extra changes bled in to remove trailing spaces and a few extra newlines in a couple sandcastles. These apparently are not formatted automatically with prettier itself so I think they came from my review process in VSCode stripping the trailing spaces when I save the files.

The merge process for those prettier changes says to use strategy=ours when merging the prettier changes. This means every PR this is merged into will likely show these set of files as changes when compared to main. This PR is just to "revert" that to make other PR reviews easier. (first noticed in b73f87e)

Issue number and link

Related to #12172

Testing plan

  • create a test branch off of another PR
  • do the prettier merge process but swap out this branch instead of origin/main
  • confirm the final diff doesn't show these changes

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have updated the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace jjspace requested a review from ggetz September 27, 2024 19:41
@jjspace
Copy link
Contributor Author

jjspace commented Sep 27, 2024

@ggetz we're potentially getting into some weird git territory here. Should this PR contain the commit to revert these changes? If that got merged to main would that prevent them from polluting other PRs? or just create more conflicts?

@ggetz
Copy link
Contributor

ggetz commented Sep 27, 2024

@jjspace Given #12206 (comment), does it make sense to close this?

@jjhembd
Copy link
Contributor

jjhembd commented Sep 27, 2024

@jjspace thanks for tracking down what the difference was... I saw too many changes when I followed the original merge process, but didn't take the time to understand why.
Whatever your VSCode did, I think it was good? So I would vote for not adding those extra spaces back in.

@jjspace
Copy link
Contributor Author

jjspace commented Sep 27, 2024

Given #12206 (comment), does it make sense to close this?

Yes, I tried again with @jjhembd's change in that branch and it worked much smoother. I'm closing this in favor of keeping the changes even if they were in addition to base prettier.

thanks for tracking down what the difference was... I saw too many changes when I followed the original merge process, but didn't take the time to understand why.

After looking closer I did notice that the merge of post-prettier-v3 in my first attempt of #12212 was actually completely empty given the strategy.

Whatever your VSCode did, I think it was good? So I would vote for not adding those extra spaces back in.

I agree, I think it's best to not have trailing spaces anywhere but apparently that's not enforced with prettier inside template strings.

@jjspace jjspace closed this Sep 27, 2024
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.

3 participants