NullVoxPopuli / ember-addon-migrator

ember addon v1 to v2 migrator
19 stars 4 forks source link

extract-tests: fix references to "dummy" when copying dummy app files #84

Closed simonihmig closed 1 year ago

simonihmig commented 1 year ago

This will fix:

stackblitz[bot] commented 1 year ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

changeset-bot[bot] commented 1 year ago

⚠️ No Changeset found

Latest commit: f0b84a191efb69235ba8f5231fb2d7db7109a755

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

simonihmig commented 1 year ago

Tests are failing, because prettier is complaining about the use of double quotes in the jscodeshift'ed output. We could run lint:fix as part of the transform (though not sure if we can rely on the lint config files to be really valid after the file juggling, so maybe better let the user do this?), or we make ember-apply use single quotes here (see docs).

@NullVoxPopuli thoughts?

NullVoxPopuli commented 1 year ago

While it's true that conversions out in the wild could have broken lint configs, I think it still valuable to test with them in the tests here.

I've PR'd to ember-apply to apply passing quotes, as well as probably all other recast options. https://github.com/NullVoxPopuli/ember-apply/pull/469

simonihmig commented 1 year ago

I think it still valuable to test with them in the tests here.

Yeah, sure, I didn't wan t to suggest we remove that. Was just unsure if our migrator should really run lint:fix, or just try to not break linting (by using single quotes)

I've PR'd to ember-apply to apply passing quotes

:tada:

NullVoxPopuli commented 1 year ago

ember-apply@2.8.0 will have the ability to review more options now :tada:

https://github.com/NullVoxPopuli/ember-apply/pull/470

simonihmig commented 1 year ago

Cool, thanks a lot. Will update this PR then...

We will just assume everyone has set up prettier to use single quotes? Or should we even try to read that from some config? But there could be so many places where that bit is stored, so I'd rather like to not have to do this! 😅

NullVoxPopuli commented 1 year ago

Yeah, that's a good point -- I think for the tests, we should run lint:fix before lint -- because for the tests, it should be safe to do -- that way we don't need to make assumptions about other people's lint / prettier configs, and maybe in the "yay you did it messaging", we can remind folks to run their lint:fix scripts?

simonihmig commented 1 year ago

I think for the tests, we should run lint:fix before lint

Ok, right. But then we don't even need the single quotes change, right? But maybe still good to have, as >90% use single quotes, so there is less to "fix" afterwards...

NullVoxPopuli commented 1 year ago

Ok, right. But then we don't even need the single quotes change, right? But maybe still good to have, as >90% use single quotes, so there is less to "fix" afterwards...

correct

simonihmig commented 1 year ago

Ok, this is working now. It is using single quotes. And I had to add the gitignore flag to the globby() calls, otherwise it would try to "fix" >32.000 files in node_modules! 🙈😅

I didn't change the tests to run lint:fix instead of lint, so to make sure this PR is working even without it. But I can do that in a separate PR if you want...