-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
chore: tests for ast-manipulation
#17
Conversation
bb4c20e
to
d630f5f
Compare
commit: |
this can be found here: https://github.com/svelte-add/svelte-add/pull/543/files#diff-a7d866907aebe643d4f4ac035b89e786ccbd073912df653bccb8ccc579a4da69 |
Thanks, that indeed fixes the test. |
ast-manipulation
ast-manipulation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall, this looks really good! just a few minor changes are needed.
adding / comiting empty input.ts files is probably useless. Removing them on the other side could cause confusion why some files have that file and some do not
im in two minds about this. on one end, i get it for the sake of consistency. on the other, it does needlessly clutter up directories.
i think im leaning more towards the latter, though. we'll be alright if they're deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perfect!
This adds initial testing capabilities to
ast-manipulation
Things to consider:
input.ts
files is probably useless. Removing them on the other side could cause confusion why some files have that file and some do notTodos:
html
utilitiescss
utilitiesdefault-export
testnamed-export
testnamed-import-merging
test (@AdrianGonz97 didn't you worked on that in the past? Would you mind linking / contributing the relevant code?)