Skip to content

Conversation

@benmonro
Copy link
Member

What: Add new rule prefer-to-have-value

Why: all the matchers!

How: with code.

Checklist:

  • Documentation
  • Tests
  • Ready to be merged

@codecov
Copy link

codecov bot commented Nov 30, 2020

Codecov Report

Merging #111 (7ead565) into master (dfe85c0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #111   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        13    +1     
  Lines          247       259   +12     
  Branches        37        37           
=========================================
+ Hits           247       259   +12     
Impacted Files Coverage Δ
src/rules/prefer-checked.js 100.00% <ø> (ø)
src/rules/prefer-empty.js 100.00% <ø> (ø)
src/rules/prefer-enabled-disabled.js 100.00% <ø> (ø)
src/rules/prefer-focus.js 100.00% <ø> (ø)
src/rules/prefer-in-document.js 100.00% <ø> (ø)
src/rules/prefer-required.js 100.00% <ø> (ø)
src/rules/prefer-to-have-attribute.js 100.00% <ø> (ø)
src/rules/prefer-to-have-style.js 100.00% <ø> (ø)
src/rules/prefer-to-have-text-content.js 100.00% <ø> (ø)
src/index.js 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dfe85c0...7ead565. Read the comment docs.

category: "jest-dom",
description: "prefer toHaveValue over checking element.value",
url: "prefer-to-have-value",
recommended: false,
Copy link
Member

@nickserv nickserv Nov 30, 2020

Choose a reason for hiding this comment

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

Why not recommend this rule? Seems safe and it's still fixable.

Suggested change
recommended: false,
recommended: true,

Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted to vet it a bit with the new smoke test feature we added. But yeah this is the plan

},

//expect(element).toHaveAttribute('value', 'foo') / Property
[`CallExpression[callee.property.name=/toHave(Attribute|Property)/][arguments.0.value=value][arguments.1][callee.object.callee.name=expect], CallExpression[callee.property.name=/toHave(Attribute|Property)/][arguments.0.value=value][arguments.1][callee.object.object.callee.name=expect][callee.object.property.name=not]`](
Copy link
Contributor

Choose a reason for hiding this comment

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

Is [arguments.1] needed here?

Copy link
Member Author

@benmonro benmonro Dec 1, 2020

Choose a reason for hiding this comment

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

yeah here's it's needed. its essentially a non null check to ensure there's a 2nd param ("foo" in the commented example.)

@benmonro benmonro merged commit 043e7bf into master Dec 2, 2020
@github-actions
Copy link

github-actions bot commented Dec 2, 2020

🎉 This PR is included in version 3.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MichaelDeBoey MichaelDeBoey deleted the feature/to-have-value branch June 12, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants