-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add lint friendly assertions #48
Conversation
I don't support this on principle. |
Which principle would that be? The fact that code with no apparent side effects is deemed bad enough form that linters throw errors on it, or do you perhaps feel that test code isn't important enough for static analysis? I'm happy to change the PR to be more in-line with your principles but I don't know how it could get more simple than a single line of code that adds a simple new feature from upstream with absolute backwards compatibility. |
It was discussed in the Chai issue. chaijs/chai#41 |
Yes, I was a part of that discussion, I'm also the one that wrote the implementation for chaijs/chai#297, which caused them to reopen chaijs/chai#41 and should be merged today. I'm confused by why you are refusing your users an arguably valuable feature just because you don't like the form, especially when the existing form still works exactly as it did before with absolutely no changes. Beside every other argument against getters with side-effects -- especially throwing exceptions --, and squelching lint errors in tests, the biggest reason I would like to remove these from my test code is because it prevents false-positives. You do realize that javascript will happily execute If this simple change is something you're patently against merging, then I'll just direct people in that thread to my fork for when they find it through a google search like I did. |
+1000 |
@domenic I'd ask you to reconsider this - especially because we have another issue on Chai related to this (chaijs/chai#272) - which adds another feature: allowing the message to be overridden on these assertions, using If you'd like, I can do the work necessary to make this a PR. It should be a really small change, and it seems like there is a good amount of users wanting it. |
If/when accepting this pull request adds new functionality in that way, I'll reconsider. Not before. |
Good to know @domenic, thanks for your work on sinon-chai 😄 |
Thanks guys for looking at this. I think both features will be a huge benefit to users, it's obviously a hot topic. |
I would appreciate it if nobody added +1s to this thread. I am leaving it un-locked because I want to let the OP amend it once the PR adds new functionality, but until then as I said about this will not be accepted. |
It kind of seems like the message thing is tangential to this feature. Not sure I see why the two need to be part of the same pull request. |
Please read the whole thread before commenting. Without new functionality, I do not agree with this pull request. Locking. |
Starting in chai 1.10.0 there are callable assertion terminators available; see chaijs/chai#297. This change checks for this functionality and uses it if available.
These both will now be valid ways to assert for
called
,calledOnce
,calledTwice
, andcalledThrice
.