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.

Sign up for GitHub

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

Jump to bottom

fix: improve support for it.each involving tagged template literals #701

Merged
merged 5 commits into from
Nov 12, 2020
Merged

fix: improve support for it.each involving tagged template literals #701

merged 5 commits into from
Nov 12, 2020

Conversation

k-yle
Copy link
Contributor

@k-yle k-yle commented Oct 28, 2020

it.each has an unusual syntax which means no-disabled-tests, no-test-prefixes, and consistent-test-it don't work for it.

This PR makes it.each work with those three rules

@SimenB SimenB requested a review from G-Rath October 28, 2020 10:15
G-Rath
G-Rath requested changes Nov 1, 2020
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Wow, thanks for this - I'm not sure how we missed it for so long!

I've left a few nits, and asked to have the interface renamed and its export removed (since its not needed); otherwise I think this is good to go!

src/rules/no-test-prefixes.ts
@@ -43,12 +49,14 @@ export default createRule({
function getPreferredNodeName(nodeName: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: a good refactor for this could be to do split('.') - then you could just splice the only/skip in as element 1, and return .join('.').

I'm fine with this how it is, so that doesn't have to be done - just thinking of the future :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'll leave this one as-is

@G-Rath G-Rath changed the title Fix bugs with it.each in many rules fix: improve support for it.each involving tagged template literals Nov 1, 2020
@k-yle k-yle requested a review from G-Rath November 1, 2020 03:34
@k-yle
Copy link
Contributor Author

k-yle commented Nov 11, 2020

hey @G-Rath, I've made the changes you requested - is there anything else I need to do?

G-Rath
G-Rath approved these changes Nov 12, 2020
View reviewed changes
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

is there anything else I need to do?

You need to prod me because I apparently forgot to approve and merge this 😅

@G-Rath G-Rath merged commit 2341814 into jest-community:master Nov 12, 2020
github-actions bot pushed a commit that referenced this pull request Nov 12, 2020
@semantic-release-bot
chore(release): 24.1.1 [skip ci]
ce08024 
## [24.1.1](v24.1.0...v24.1.1) (2020-11-12)

### Bug Fixes

* improve support for it.each involving tagged template literals ([#701](#701)) ([2341814](2341814))
@github-actions
Copy link

🎉 This PR is included in version 24.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@k-yle k-yle deleted the bugs-with-each branch November 12, 2020 08:22
@shobhitsharma
Copy link

@k-yle getting some error Return a Promise instead of relying on callback parameter jest/no-done-callback after this upgrade. Maybe more info for this change would help.

@k-yle
Copy link
Contributor Author

k-yle commented Nov 12, 2020

@shobhitsharma yep looks like this PR broke jest/no-done-callback for it.each 🙊 working on it right now

@k-yle k-yle mentioned this pull request Nov 12, 2020
Merged
@k-yle
Copy link
Contributor Author

k-yle commented Nov 12, 2020

fixed in #708

cc @G-Rath

SimenB added a commit that referenced this pull request Nov 12, 2020
@SimenB
Revert "fix: improve support for it.each involving tagged template li…
5cc1d1b 
…terals (#701)"

This reverts commit 2341814.
@SimenB SimenB mentioned this pull request Nov 12, 2020
Merged
@SimenB
Copy link
Member

SimenB commented Nov 12, 2020

@k-yle @G-Rath (is it a new zealand thing to go with that structure for usernames? 😀) I've revert this change in #713, but kept all the passing tests (plus an extra regression test for #710). A new go would be great 👍

@k-yle
Copy link
Contributor Author

k-yle commented Nov 13, 2020

@SimenB I won't have time until next Tuesday so if someone else wants to do it before then, go for it 👍

it may also be above my skill level to fix this edge case - I don't know how no-done-callback could work with that syntax

also, do we know how to reproduce #711?

@k-yle k-yle mentioned this pull request Nov 27, 2020
Merged
@Obi-Dann Obi-Dann mentioned this pull request Apr 10, 2021
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimenB SimenB SimenB left review comments

@G-Rath G-Rath G-Rath approved these changes

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@k-yle @shobhitsharma @SimenB @G-Rath