All assertions should be methods

[aseemk]

I love the clever use of getters in should.js, but one request: the API would feel cleaner and test code would be more understandable/maintainable/readable if all assertions were methods.

E.g. foo.should.be.ok; as a standalone statement reads like a bug to me in the same way that window.navigator.userAgent; as a standalone statement does. I instead want to be able to write foo.should.be.ok().

I realize there's a lot of should code that would break if the current way didn't keep working. But I think it's possible to support both this new style and the older style:

• have the Assertion object inherit from Function, so that it can be called; and
• have calling it do nothing (so the assertion still happens in the "get" of the ok property) and return itself (arguments.callee or this, not sure if there would be a difference).

What do you think? Am I missing something? I've made a fork and am ready to try this out; will let you know if I succeed or fail. =)

Cheers, Aseem

[edited language to be a bit clearer]

[aseemk]

An advantage to methods is you can pass an optional message argument -- see my comment in issue #1. But if we want assertions to be aware of and use this message, we can no longer assert on the get of the property; we would need to assert on the function call.

So I think the long-term ideal thing to do is indeed break foo.should.be.ok, but for the time being, I see two options:

• Delay the assert via process.nextTick() in the getter of ok, and cancel that delay if ok() is called. The upside to this is that messages work as you'd expect; the downside is that existing code's assertion errors will now fire async, so you lose the stack trace. But maybe we could still internally create a helpful message for the user.
• Make message support for ok etc. opt-in via a config. E.g. should.useMessages() or should.useMessages = true or something. (Not sure what a good name is.) This way, existing code continues to work as before, and new code can opt-in. But of course it's lame having to know about this opt-in or write boilerplate code.

I'm going to try my stab at the first option above. Will keep you posted.

[aseemk]

Sweet, good news -- we can indeed "remember" the stack trace if we want to delay the assertion and the caller doesn't call the method:

> try { require('assert').ok(false); } catch (e) { console.log(e.stack); }
AssertionError: true == false
    at [object Context]:1:25
    at Interface.<anonymous> (repl.js:150:22)
    at Interface.emit (events.js:42:17)
    at Interface._onLine (readline.js:132:10)
    at Interface._line (readline.js:387:8)
    at Interface._ttyWrite (readline.js:564:14)
    at ReadStream.<anonymous> (readline.js:52:12)
    at ReadStream.emit (events.js:59:20)
    at ReadStream._emitKey (tty_posix.js:286:10)
    at ReadStream.onData (tty_posix.js:49:12)

TJ, do you know if there's any other downside to delay-asserting on property-gets only?

E.g. currently, false.should.be.ok; asserts right away. After my change, false.should.be.ok; will assert asynchronously. Do you see any problem w/ that?

Thanks!

[tj]

it's not necessarily a problem, but most test frameworks have a tough time mapping the origin of an exception without the original stack, and something that looks like it executes immediately, but doesn't could get confusing

[DanielBaulig]

Shouldn't the message stuff be discussed in a different topic? Yes, adding the option to actually call an assert explicitly would open up this option, but the fact that true.should.be.ok; looks like a bug and true.should.be.ok() throws, needs to be discussed independently. Not only do those asserts look like bugs, but most editors and IDEs will infact highlight them as possible bugs and at least issue a warning.

Using arguments.callee should be avoided. Accessing it in ES5 strict will throw.

[tj]

if a bunch of properties looks like a bug to your IDE, something is wrong lol

[DanielBaulig]

Yeah, but not with the IDE. Expression statements are non-sense and shouldn't be in the language at all. Seeing that Brandon Eich and Douglas Crockford agree with me on that matter, I feel in good company with this argument. If a syntax parser encounters a expression statement it is more often than not a programming error and this it is absolutely correct to issue a warning.

[tj]

every language with expressions has an expression statement lol...

[DanielBaulig]

I dont want to argue with you about this, but in fact there are programming languages that seperate expressions and statements very strongly and do not allow both interchangeably like C-style languages do, eg Pascal. So not every language has expression statements.

The point is, as assemk pointed out correclty, that you cannot see from code alone if true.should.be.ok; actually is a programming error (with missing parantheses) or the way it should be used. Most people reading this construct without knowing the ins and outs of should.js would be confused at least. And it is the reason why JSLint warns you if it finds this construct in your sourcecode. You cannot tell without knowledge of a lot of semantics if this is a programming error or the way it was intended to be.

This is a suggestion on how to improve should.js' API, make it more streamlined and reduce friction with syntax checkers like JSLint. Take it, or leave it.

[aseemk]

FYI, I already made this change in my fork, along with several others. I've been planning on continuing this work but am just a bit busy with a project at the moment. =)

[tj]

@DanielBaulig if you have an expression that is valid as a statement, then it is an expression statement. js has them, so why are you claiming it's invalid syntax? its not an error in any respect. It's not my fault JSLint sucks

[DanielBaulig]

I do not claim its invalid syntax (and so doesn't JSLint), it's perfectly fine from a language perspective. But, except for this special use-case in should.js, (and the border case of a pure function call statement, which - technically - is also an expression statement in ES*) give me a valid reason why you would want to do expression statements in your code? There are only very few, at most. This is the reason that eg. JSLint warns you if it finds an expression statement. It is very uncommon, that the expression statement was intended. More likely the programmer forgot paranthesis for a function call or screwed up with a semicolon somewhere. For this reason issueing a warning to the programmer is absolutely appliceable and the right thing to do. This has nothing to do with JSLint or any other syntax parser sucking, but writing clean, easy to read and unambiguous code.

• Technically simple function calls are also expression statements in ES. Since ES allows expression statements there is no need for an pure statement subroutine call. Pascal for example has the procedure construct additionally to a function, which is a pure expression subroutine call.

[tj]

like I said, dont rely on JSLint, it's not my issue :p

[DanielBaulig]

Well, if code quality is not your issue, then I will have a hard time convincing you that using expression statements is a bad habbit. But if code quality happens to be an issue for you, you should reconsider your position on this matter. You loose nothing by merging in the patch suggested by aseemk, but gain alot of code expressiveness and cleaner test suites. I think I provided plenty of good arguments to do so.

Keep up the good work.

[tj]

its more a language issue. I dont disagree that ambiguity is not great, it's pretty odd that people are trying to get proxies in and all of Brendan's talk about overloading operators n such, not good :s. when I have a chance I'll review/merge these

[robinheghan]

I have to agree on this. A property should return a value, not do something. This is the case in almost all C-like languages. If you expect a statement to do something, it should have parentheses to reflect that.

[tj]

test frameworks are one of the few exceptions IMO where they can be "cute", it's also ambiguous and weird/gross to be using accessors to pass the object back for chaining etc

[paulmillr]

Nope, big and fat −1 for this proposition. If your IDE doesn't support js getters, then it's shit.

Should.js is beautiful. Sadly, IE doesn't support it, but there is expect.js for IE etc.

[robinheghan]

@paulmillr this shouldn't be a case of if IDE support, rather what the developer expects something to do. Anything that DOES something, should be a method period. A getter should only be used if it only, ONLY, returns something. An assertion doesn't return anything, it DOES something and therefore should be a method. If it really gross'es you out when it comes to chaining, it really isn't a problem to write two statements, or break the one statement into two or more lines.

While I agree that Should.js in general is great, it.should.follow.best.practices.for.programming. The very fact that .ok was a getter made me trip up when I started because, as .ok is an assertion, i thought it was a typo in the doc that parentheses was left out, and I'm sure other programmers thought this as well.

[TooTallNate]

This is an interesting thread.

While I tend to agree with this thread, that the getter assertions are a little weird in Should, what's being recommended is a big breaking change that will silently break all Should test cases currently out there. Silent, because when the getter turns into a function, the test case will simply access the function and then never invoke it, so the test case won't throw any assertion there, making it seem like it's passing when it may not be.

What I personally don't like about the getters is that I sometimes would use the wrong property name, thinking an assertion was being performed, when really I was just accessing undefined. But this, IMO, was only minor, and a simple solution for me was to just have the Should README open in a tab while writing tests for reference.

So overall TJ might have tried to be too "cute" with the getters, but it would also be hard to make the change, knowing that users won't just blindly upgrade and have their tests silently fail. So.... interesting....

[robinheghan]

@TooTallNate then again, Should.js have yet to reach 1.0, which not only means that breaking the API is allowed, but expected...

[DanielBaulig]

I still think it.should.be.ok(); @TooTallNate There could be a transition period with an opt-in. The property would remain, but would return a function instead of calling it, if a special flag was set.

Especially in a testsuite expressiveness should trump "cuteness" any day. Bar the subjective discussion if should.be.ok; actually is "cute". But I guess all arguments have been made. If TJ decides not to increase the expressiveness of should.js for satisfying his (and maybe others?) notion of a cute API then it's how it is.

[tj]

`it.should().be().equal(foo)`` ;) haha

[aseemk]

Hehe, came across this thread again. Thanks for the reasoned and objective response, @TooTallNate.

My only reaction was that this change doesn't have to lead to silent failures; I proposed above that assertions could just be triggered on next tick if they weren't triggered on the tick that they were set.

E.g. this would be the new "correct" style:

foo.should.be.ok();

But old code would still work:

foo.should.be.ok;

Accessing .ok would (a) return a function that would assert when invoked, but also (b) register a process.nextTick() event to invoke that function if it hadn't been yet.

Funny that it's been over a year since this thread was started! =P

Edit: And the library could even console.warn() on that process.nextTick(), letting the developer know.

[nearstate]

@aseemk - wouldn't your suggestion restrict the use of should to node?

Compromise (for all you old-skool jslinters out there (like me)):

var assertion = foo.should.be.ok;

[aseemk]

Good catch @nearstate, but using setTimeout instead of process.nextTick in the browser would work and achieve the same effect.

[gasi]

Haha, found this thread after looking for ways to define messages for should.be.ok.

[aseemk]

@gasi: support for messages (via assertions as methods instead of getters) has been in my fork:

https://github.com/aseemk/should.js/blob/master/aseemk.md

My fork is stable, but doesn't have the newer features of this upstream since v0.4 (inclusive):

https://github.com/visionmedia/should.js/blob/master/History.md#040--2011-12-16-

But on a related note, I've moved to using expect.js instead of should, FWIW, and it uses methods for assertions:

https://github.com/LearnBoost/expect.js

It's not there yet, but @guille is also open to having custom messages in this same way:

https://github.com/LearnBoost/expect.js/issues/18

Unfortunately, expect.js doesn't seem to be as feature-rich as should. Alas, I'm getting by okay. =)

[NotMyself]

Found this thread look for a way to make jshint happy along with how to write messages on assertion failure. Going to check out expect.js. Thanks @aseemk.

[tj]

im +1 for the changes, we can just bump major, all the more reason getters/setters shouldn't exist :D