JamesMGreene / qunit-assert-html

A QUnit plugin for assertion methods to compare two HTML strings for equality after a rigorous normalization process.
MIT License
13 stars 5 forks source link

Extend QUnit.assert instead of QUnit. #3

Closed Krinkle closed 11 years ago

Krinkle commented 11 years ago

I don't think we've ever documented accessing assertion methods via the QUnit object.

They've been global (as primarily now) and as methods on the assert object, but not on the QUnit object.

I'd recommend putting them on the assert object and perhaps exposing them on window and/or QUnit for compatibility.

Though perhaps, as being a relatively new plugin, you could drop the compatibility and encourage people to use the new pattern.

JamesMGreene commented 11 years ago

I'm totally happy to change this paradigm, I was just following what was done for the existing bundled assertion addons (canvas, close-enough, step), so we need to file issues against those addons as well.

So, if I'm understanding you correctly, I should change my:

QUnit.extend( QUnit, { /* ... */ });

to

QUnit.extend( QUnit.assert, { /* ... */ });

Correct? No other code changes unless I want to continue perpetuating the QUnit.{assertionMethod} or window.{assertionMethod} paradigm, right? (And obviously I'd need to update all of the QUnit.{assertionMethod} references to be QUnit.assert.{assertionMethod} instead.)

JamesMGreene commented 11 years ago

Just submitted a PR to fix the bundled QUnit addons in this same manner.

Krinkle commented 11 years ago
And obviously I'd need to update all of the QUnit.{assertionMethod} references to be QUnit.assert.{assertionMethod} instead.

No, that's not obvious and actually "wrong" in my opinion. Making these assumptions is exactly how wildcard incompatibilities are born. The definition is in QUnit.assert, but references from usage should never ever refer to QUnit.assert as it is fundamentally defeating the designed pattern, which is about context.

This is like providing a plugin $.fn.addStuff, and advertising usage by doing $.fn.addStuff.call([element], 'stuff');.

The new pattern is about not using globals and using context.

So, if you want to drop the old pattern (which is a choice you can make), then go this way:

/* Plugin */

var plugin = {
    equalMoreStuff: function () { ... }
};
$.extend( QUnit.assert, plugin );

/* Usage */

assert.equalMoreStuff( .., .. );

/* Example */

QUnit.test( 'name', 4, function ( assert ) {
    assert.equalMoreStuff( .., ..);
} );

Or, go the old way:

/* Plugin */

var plugin = {
    equalMoreStuff: function () { ... }
};
$.extend( QUnit.assert, plugin );
$.extend( QUnit, plugin );
$.extend( window, plugin );

/* Usage */

equalMoreStuff( .., .. );

// Alternative, common when using non-browser environments
QUnit.equalMoreStuff( .., .. );

/* Example */

test( 'name', 4, function () {
    equalMoreStuff( .., ..);
} );
test( 'name', 4, function () {
    QUnit.equalMoreStuff( .., ..);
} );

Please erase any hybrids or and self-invented look-a-likes such as using QUnit.assert.... as soon as possible to avoid spread.

Right now using it directly might work, but it is unsupported and will actually break in the near future when it becomes a a context-bound object.

JamesMGreene commented 11 years ago

@Krinkle Thanks for clarifying, I had misunderstood what you said before. I'll remove the QUnit.assert.{assertionMethod} examples and usage ASAP.