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

Escape backslashes from the paths in the generated code #13

Merged
merged 2 commits into from
Dec 2, 2024

Conversation

JD557
Copy link
Contributor

@JD557 JD557 commented Dec 1, 2024

This is required for the library to work on Windows.

Otherwise, it would fail with errors like:

[error] 2 |object Snapshots extends com.indoorvivants.snapshots.Snapshots(location = "D:\Projects\cue4s\modules\cats-effect\src\snapshots\catsEffectJS", tmpLocation = "D:\Projects\cue4s\modules\cats-effect\target\js-3\resource_managed\test\snapshots-tmp", forceOverwrite = false)
[error]   |                                                                                             ^
[error]   |                                                  invalid escape character

@keynmol
Copy link
Contributor

keynmol commented Dec 1, 2024

Thanks, Windows strikes again :D

Are there no APIs on windows JVM that produce a good string out of File/Path? That's a shame.

As part of this, do you think it's possible to turn the build into a matrix build (here: https://github.com/indoorvivants/snapshot-testing/blob/main/.github/workflows/ci.yml#L13) running on windows as well?

Frankly I completely missed this being a problem

@JD557
Copy link
Contributor Author

JD557 commented Dec 1, 2024

Are there no APIs on windows JVM that produce a good string out of File/Path? That's a shame.

Yeah, I think this is a particularly hairy case, because the String is technically correct, but needs to be "double escaped", as it's pasted straight into the code.
I know that there's a escapedString function but:

  • It's private
  • It's Scala 3 only

As part of this, do you think it's possible to turn the build into a matrix build (here: https://github.com/indoorvivants/snapshot-testing/blob/main/.github/workflows/ci.yml#L13) running on windows as well?

I think so, and I imagine it's quite easy, but I think it might be easier for you to do it (feel free to push commits to this PR).
I am not able to trigger the workflow without pinging you (see the screenshot), so I have no easy way to check if I messed something up 🙃

imagem

@keynmol keynmol merged commit 961a20c into indoorvivants:main Dec 2, 2024
2 checks passed
@keynmol
Copy link
Contributor

keynmol commented Dec 2, 2024

Welp, I tried to add windows: https://github.com/indoorvivants/snapshot-testing/actions/runs/12115271668/job/33773314295#step:4:602

The resulting error looks worse than what I have motivation for right now..

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