avajs / ava-codemods

Codemods for AVA
https://ava.li
MIT License
68 stars 16 forks source link

Add transform from Tape to AVA #28

Closed skovhus closed 8 years ago

skovhus commented 8 years ago

Please have a look @sindresorhus @DrewML

For background, see https://github.com/avajs/ava-codemods/issues/5

Missing

Closes #5

sindresorhus commented 8 years ago

@DrewML Since you started working on this, would you be able to help review this pull request?

DrewML commented 8 years ago

@sindresorhus No problem. I'll take a look in a few hours.

DrewML commented 8 years ago

Looks good. Awesome work @skovhus!

skovhus commented 8 years ago

@DrewML thanks! Good feedback.

Any thoughts on CLI integration? Does it even make sense? People can also just run jscodeshift directly instead of us wrapping their CLI.

DrewML commented 8 years ago

Any thoughts on CLI integration? Does it even make sense? People can also just run jscodeshift directly instead of us wrapping their CLI.

That's a good question. I think one of the reasons for this repository having a CLI integration is to add in some safety checks (mainly making sure working branch is clean).

It also looks like the CLI is for the ease of supporting codemods for multiple versions of AVA, but this transform is obviously different since this is the first transform that isn't AVA >> AVA.

@sindresorhus @jamestalmage thoughts?

skovhus commented 8 years ago

Thanks for the thorough review @DrewML!

So except for the CLI integration and maybe docs update I think we are good.

sindresorhus commented 8 years ago

Yeah, CLI integration would be best, both for safety and ease for the user. It's nice to be able to tell users just to install and run ava-codemods instead of forcing them to learn about codemods and how to use jscodeshift. CLI integration is not a requirement for this PR to land. We can tackle that later if you prefer?

As for CLI, I was thinking $ ava-codemods --tape that would launch the Tape migration instead of the version upgrade. Alternatively we could expose a separate binary called tape-to-ava. Thoughts?

skovhus commented 8 years ago

A separate binary seems pretty clean to me. It has a different purpose than migrating from AVA versions. 👍

sindresorhus commented 8 years ago

A separate binary seems pretty clean to me. It has a different purpose than migrating from AVA versions. 👍

Alright, let's do it.

skovhus commented 8 years ago

@sindresorhus @DrewML except README update (and the two ideas for later versions mentioned above), I think this transformer should be ready to be merged. 😺

I would like to squash the commits after you have reviewed.

Have a look at the simple CLI wrapper for jscodeshift here https://github.com/avajs/ava-codemods/pull/28/commits/7a23f7d09787e93dda61b489e1436e9a9a66b928

sindresorhus commented 8 years ago

I think this is pretty good to land except for some nitpick I've commented on and updating the readme.

I'll give the rest of the team (@avajs/core) a couple of days to review.

sotojuan commented 8 years ago

I think this is awesome work!

skovhus commented 8 years ago

Thanks @sotojuan !

@sindresorhus I think I have covered your changed and tried updating the README. Let me know what you think.

Any preference in me squashing the commits before merge?

sotojuan commented 8 years ago

I think we (@sindresorhus mostly) squash ourselves before committing, but I'll let him decide.

jfmengels commented 8 years ago

@skovhus This is awesome, thanks a lot for working on this!

For the squashing part: Yes, don't worry about it, the merger will take care of it.

Following https://github.com/avajs/ava-codemods/pull/29, we are using a different tool to create tests, but this can be done later and I don't mind doing it (and there might be a bit of work to do to make that tool compatible with the tests that need to be run sequentially).

I always had in mind that you'd have on executable for this repo, and that it would ask you the question of what you wanted to do: "Upgrade AVA to a new version" or "Transform from a different test framework", where you could then select the test framework you came from (assuming we supported more than 1). This is absolutely not blocking, as I think that this is fine, but it is probably worth putting it back into only one cli later when we have more options.

Again, thanks a lot @skovhus, this is really great work!

10i1feE6ZBwHcY

skovhus commented 8 years ago

@jfmengels thanks! 😄

I've updated to use the the same testing as in #29 ...

Rebased and ready for merge @sindresorhus

vadimdemedes commented 8 years ago

@skovhus Incredible work, thank you!

giphy 6

skovhus commented 8 years ago

Thanks @vdemedes ! Codemods are fun! 😸

skovhus commented 8 years ago

@sindresorhus let me know if you need anything from me to land this. : )

skovhus commented 8 years ago

I've rebased with master.

sindresorhus commented 8 years ago

This is truly awesome @skovhus \o/

happy

skovhus commented 8 years ago

🎉 🎉 🎉

Thanks @sindresorhus !