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
Should 'prefer-to-have-count' be merged into the rule 'prefer-web-first-assertions'? #289
Comments
Hi @mskelton, sorry to bother you again, but what do you think of this suggestion? |
@An631 You opened this issue 20 minutes ago and then commented 4 minutes later with a "hey, did you see this" type of comment. I can't assist within 4 minutes of you opening an issue, I have a full time job and work on this plugin in addition to that. Comments like the one you added really don't help open source maintainers be motivated to continue maintaining software as comments like that are just spam. |
Oh, sorry if I came across incorrectly with my comment. I am not sure how the notifications for maintainers are setup (never had a repo of my own) I was just trying to bring attention to the issue for whenever you have time to look at it as I see you as the owner of the file/rule and I am not sure how new issues get notified to the maintainers. |
@An631 With most open source projects (especially projects that are actively and frequently maintained), you can assume that maintainers will have notifications of some sort for new issues that are opened. In general with open source projects, it's really best to try and avoid comments like "Hey, did you see this" or "what's the status of this" type of thing. Sometimes it's necessary if something is pending for a long time with no comments or you have a PR that the maintainers haven't commented or reviewed on in a while, but in general, maintainers will thank you if you can keep those kind of comments to a minimum 😁 |
Anyway, sorry to get sidetracked, I like your suggestion in principle, definitely feels like it could go with prefer-web-first-assertions. That said, it would be a breaking change, so not sure if the juice is worth the squeeze. I'll leave this issue open, and when we get to other breaking changes I might consider this in that batch. |
Thanks for the explanation! I'll make sure to not make comments like those going forward :) Regarding the change: I agree with the breaking nature of it and makes sense to postpone it until more breaking changes are required. I am happy to help with the change once we get to that point, so feel free to ping me if you find yourself needing more hands to make it happen 👍. Thanks! |
Right now we have a separate rule that checks for
count()
and recommends usingtoHaveCount()
instead. Not sure if there is a reason for this differentiation or special treatment for this assertion given thattoHaveCount
is part of the web-first assertions that are recommended by Playwright and we already have a rule covering those: 'prefer-web-first-assertions'.I am noticing that some of the issues that I fixed in the 'prefer-web-first-assertion' rule are also failing for the
toHaveCount()
rule.For example, the arguments of the
expect
call are not verified to see if they were created using thecount()
function or not, and only theexpect
calls that contain acount()
method directly inside it and only those that include anawait
are flagged as seen in this screenshot:Given that
toHaveCount
is also a web-first assertion, should we consider getting rid of this special rule and just add it to the set of checks prefer-web-first-assertion is doing? This way we can benefit from all the fixes and edge cases that have already been considered for the rest of web-first-assertions.The text was updated successfully, but these errors were encountered: