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

feat(await-async-event): add basic fixer #656

Merged
merged 1 commit into from
Oct 2, 2022
Merged

feat(await-async-event): add basic fixer #656

merged 1 commit into from
Oct 2, 2022

Conversation

skovy
Copy link
Collaborator

@skovy skovy commented Oct 1, 2022

Checks

  • I have read the contributing guidelines.
  • If some rule is added/updated/removed, I've regenerated the rules list (npm run generate:rules-list)
  • If some rule meta info is changed, I've regenerated the plugin shared configs (npm run generate:configs)

Changes

  • Add fixer for await-async-event rule

I was thinking if this fixer should update the it/test to also be aysnc if not, but that felt like out of the scope of this rule/fixer? It cares if it's awaited, not managing if it's an async function wrapping?

Context

Branched off and a follow-up to #652 to make the migration easier to user-event v14.

Related to one of the rules mentioned in #202.

@MichaelDeBoey MichaelDeBoey changed the title feat: await-async-event basic fixer feat(await-async-event): add basic fixer Oct 1, 2022
@MichaelDeBoey MichaelDeBoey marked this pull request as draft October 1, 2022 18:27
@Belco90
Copy link
Member

Belco90 commented Oct 2, 2022

Waiting until #652 gets merged (you could point this PR to the branch from that PR to make explicit it's branched off from there).

I'm happy without managing the async operator from the outer function. I think other autofixes from this plugin work that way.

@skovy
Copy link
Collaborator Author

skovy commented Oct 2, 2022

you could point this PR to the branch from that PR to make explicit it's branched off from there

The only branch options are the ones in this repo, I think since that pull request is a branch from my fork I can't base it on that branch? I can wait for that one to be merged and I can clean up this PR after.

@skovy skovy marked this pull request as ready for review October 2, 2022 19:17
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

@skovy thank you for adding the autofix implementation for this rule!

@Belco90 Belco90 merged commit 3fae55d into testing-library:alpha Oct 2, 2022
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

🎉 This PR is included in version 6.0.0-alpha.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@epmatsw
Copy link

epmatsw commented Oct 6, 2022

Fwiw, I think this fixer should either 1) convert the function to async or 2) refuse to fix the issue if the function isn't async. As-is, it just produces syntactically invalid code and requires manual attention in those cases anyways.

@Belco90
Copy link
Member

Belco90 commented Oct 7, 2022

Fwiw, I think this fixer should either 1) convert the function to async or 2) refuse to fix the issue if the function isn't async. As-is, it just produces syntactically invalid code and requires manual attention in those cases anyways.

Actually… I checked again our current fixable rules and there is none adding the await operator (I don't know why, but I assumed there was one at least already). So I agree with @epmatsw: the fixer should add the async to the outer function (if found).

It should be fairly easy to do this. What do you think @skovy?

@Belco90
Copy link
Member

Belco90 commented Oct 11, 2022

I've created #674 for improving the fixer. @epmatsw thanks for your thoughts on this!

@skovy skovy deleted the pr/await-async-event-fixer branch October 11, 2022 15:40
@skovy
Copy link
Collaborator Author

skovy commented Oct 11, 2022

Thanks @Belco90 - I'll try to take a look at the issue over the next few days/weeks if someone doesn't get to it before that 👍

@epmatsw
Copy link

epmatsw commented Oct 11, 2022

No problem! Thanks @skovy and @Belco90 for the hard work, can’t wait to get started using this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants