feat: add jest-dom-prefer-enabled-disabled, jest-dom-prefer-required, jest-dom-prefer-checked rules

[benmonro]

implements #30

[emmenko]

Nice, looks good to me!

[Belco90]

There are few things to improve in this first approach, I've left some feedback. Also, I think we must prefix all these rules related to jest-dom by jest-dom-* so this rule should be renamed to jest-dom-prefer-enabled-disabled. Additionally, you have to include the rule in the README file within Supported Rules table.

[benmonro]

@Belco90 all your requests have been addressed.

[Belco90]

Looking good so far. Some additional feedback.

[thomaslombart]

Thanks for the PR! 🙂
I spotted some missing tests and typos.

[benmonro]

alright guys, addressed the feedback.

[Belco90]

Awesome, LGTM now!

[thomaslombart]

LGTM 👏

[emmenko]

LGTM! 👌

[benmonro]

cool, i don't have permission to merge (yet? 😉)

[thomaslombart]

I tested your branch locally and everythings works fine! However, all code is not covered so here are two missing tests to add to make sure all code is covered:

Valid:

 `expect(element).not.toHaveProperty('checked', true)`,

Invalid:

 { code: 'expect(element).not.toBeDisabled()', errors: [ { message: 'Use toBeEnabled() instead of not.toBeDisabled()', }, ], output: 'expect(element).toBeEnabled()', },

When done, I'll merge the PR! 😉

[benmonro]

@thomlom @Belco90 I decided to add the prefer checked and prefer required rules as well. The reason I included them in this PR (then re-titled the PR) is that I extracted the rule for all 3 to share the same code. So probably best if you give another review. I will add docs later today.

[Belco90]

@benmonro thanks for your work for those additional rules, but please avoid this kind of "controversial" changes as now you are adding lot of stuff out of the scope of the original issue. Also, if one one those new rules added doesn't pass the review, it will block the other from being released. Better to have rules on their own PR to be discussed and reviewed individually. Also you have added more changes related to test coverage threshold that should not be involved here and done in a separated task for it.

So could you leave this PR as before and address those new rules and test coverage threshold in a different one?

[benmonro]

I had a feeling you were going to say that. I'm doing this in my spare time and don't have the time for that right now. Sorry. The code works and I was dinged for not having coverage before so now it does have it. If you want to close the PR feel free. No hard feelings.

On Sun, Oct 27, 2019 at 10:22 AM Mario Beltrán Alarcón < ***@***.***> wrote: @benmonro <https://github.com/benmonro> thanks for your work for those additional rules, but please avoid this kind of "controversial" changes as now you are adding lot of stuff out of the scope of the original issue. Also, if one one those new rules added doesn't pass the review, it will block the other from being released. Better to have rules on their own PR to be discussed and reviewed individually. Also you have added more changes related to test coverage threshold that should not be involved here and done in a separated task for it. So could you leave this PR as before and address those new rules and test coverage threshold in a different one? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBPBFC5LJ2EH3XUZA7LP3QQW537ANCNFSM4JEODFPQ> .

-- Ben Monro Software Developer

[Belco90]

I don't want to close this PR at all, just make sure everything is consistent and done following a workflow that make sense for everyone :)

No rush into doing that now, I also created and maintain this plugin in my free time: no one is putting pressure on you. Just create a new branch from this current one (for another PR) and revert this one to the previous commit would be fine, it was already approved just waiting to be merged.

[benmonro]

@Belco90 thanks for the feedback. I decided to move this feature to its own plugin: eslint-plugin-jest-dom. after more thought, it really does deserve it's own plugin, also there could be potential users who might want to use jest-dom apart from testing-library. Thanks again for the feedback and keep up the good work, i <3 this plugin.

[thomaslombart]

Hey @benmonro, it's too bad and frustrating that this feature has been moved to its own plugin. These rules are worthwile and it's relevant to include them in this plugin since we can create a shareable configuration for jest-dom just like we did for Angular, React and Vue.

I have the feeling you were frustrated by the review comments. I hope that's not the case since it wasn't the goal at all. I'm curious to have your feedback on this 🙂

Thanks for the discussion anyway!

[benmonro]

no, that's not the case at all. I still ❤️ this plugin and will still contribute when I come across things we need. There were 2 main factors in moving it to it's own plugin. 1) jest-dom isn't actually a testing library, even though it's testing library-adjacent. 2) after looking at the fact that each rule had to be prefixed with jest-dom- it seemed as though the code itself was begging for it.