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

fix: allow for whitespace in snippets declaration #2366

Merged
merged 2 commits into from May 13, 2024

Conversation

paoloricciuti
Copy link
Contributor

@paoloricciuti paoloricciuti commented May 5, 2024

This should close sveltejs/svelte#11478

The problem was that the generic generation was relying on the start position of typeAnnotation + 1. But if there are whytespaces this is not correct and the generate type is wrong Snippet<[: string]>. However inside the first typeAnnotation there's always a second typeAnnotation field which only includes the actual types (at least from what i was able to see, i wasn't able to find a type that doesn't generate at least two level deeps typeAnnotation).

This seems to work fine but give it a good look since it's the first time i'm working in this repo.

@paoloricciuti
Copy link
Contributor Author

Umh...i don't know if i've fucked up the expected file here...it seems strange that is a new one.

@jasonlyu123
Copy link
Member

jasonlyu123 commented May 6, 2024

This is because the generated snapshot is always written to the v5 file but the expected file can be both. You can move the content to the expetectv2.js file and remove the v5 file. We can tweak the snapshot generation in a separate PR. But It's probably a small change so if you're interested in tweaking it, I wouldn't mind if it's done in this PR.

@paoloricciuti
Copy link
Contributor Author

We can tweak the snapshot generation in a separate PR. But It's probably a small change so if you're interested in tweaking it, I wouldn't mind if it's done in this PR.

How it should be tweaked specifically?

@dummdidumm
Copy link
Member

TBH I'm not sure there's a good way to tweak it, because the test generation can't know if the code deviates sufficiently enough from expectedv2 to warrant a new file or not. I would just leave it as is and copy the new v5 contents into the v2 file and then delete the v5 file.

@paoloricciuti
Copy link
Contributor Author

TBH I'm not sure there's a good way to tweak it, because the test generation can't know if the code deviates sufficiently enough from expectedv2 to warrant a new file or not. I would just leave it as is and copy the new v5 contents into the v2 file and then delete the v5 file.

Fixed it

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

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

Thank you!

@dummdidumm dummdidumm merged commit 80622df into sveltejs:master May 13, 2024
3 checks passed
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.

Svelte 5: Whitespace problem in typed parameters to snippets
3 participants