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(prefer-find-by): stop prefer-find-by fixer to break code when no … #472

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

dyatko
Copy link
Contributor

@dyatko dyatko commented Sep 8, 2021

…spaces before bracket

Checks

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

Changes

Add check for spaces before brackets for prefer-find-by fixer

Context

The rule fixer didn't assume that some codebases may not separate brackets with spaces, so it's braking the code:

-        const {getByLabelText, getByTestId, getByText} = await renderComponent();
+        const {getByLabelText, getByTestId, getByTex, findByText} = await renderComponent();

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.

Hi @dyatko!

Good catch! And thank you very much for opening a PR with the fix, I really appreciate it.

It looks good to me, just a small question about if we could make it even more generic. Don't sweat it if it seems an overengineering solution, the current version is better than before!

lib/rules/prefer-find-by.ts Outdated Show resolved Hide resolved
@Belco90 Belco90 added the bug Something isn't working label Sep 8, 2021
@dyatko dyatko requested a review from Belco90 September 8, 2021 15:27
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.

Great! Thanks again for your contribution.

@Belco90
Copy link
Member

Belco90 commented Sep 8, 2021

Getting really weird errors in the pipeline when running the tests. I'll have to take a look, no idea what's happening at first sight.

@dyatko
Copy link
Contributor Author

dyatko commented Sep 8, 2021

@Belco90 yeah, same issues in another PR #465

@Belco90
Copy link
Member

Belco90 commented Sep 8, 2021

@Belco90 yeah, same issues in another PR #465

That's a different issue actually: you can see how it's failing to try to install some dependencies.

Our error is caused by Node v16.9.0: nodejs/node#40030

We need to avoid this version meanwhile and stick to v16.8.0. I'm gonna create a PR to stick that Node version our GitHub Actions, so we can get that change here.

@Belco90 Belco90 mentioned this pull request Sep 8, 2021
3 tasks
@Belco90
Copy link
Member

Belco90 commented Sep 8, 2021

Pull request created: #473

@Belco90 Belco90 merged commit 9624a44 into testing-library:main Sep 9, 2021
@github-actions
Copy link

github-actions bot commented Sep 9, 2021

🎉 This PR is included in version 4.12.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Belco90
Copy link
Member

Belco90 commented Sep 9, 2021

@all-contributors please add @dyatko for bug and code

@allcontributors
Copy link
Contributor

@Belco90

I've put up a pull request to add @dyatko! 🎉

@dyatko dyatko deleted the fix-prefer-by branch September 9, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants