browserify / commonjs-assert

Node.js's require('assert') for all engines
MIT License
293 stars 57 forks source link

[doc][readme] Does commonjs-assert aim to be an assert module as per CommonJS UT spec? #27

Closed mk-pmb closed 7 years ago

mk-pmb commented 7 years ago

As I found in #26 , the current Readme isn't clear about the intent of this module:

The API is derived from the commonjs 1.0

Well, "derived" is a weak term indeed,

Could you therefor extend that readme statement to clarify whether commonjs-assert aims to be an assert module according to the CommonJS Unit Testing 1.0 spec?

The module name (Edit: nope, just the repo name and package description) and line 45 of assert.js hint at "yes". Hints at "no" include the other issue's discussion and, more importantly, the code comment for rule 7.3 about RegExps. I can't find any mention of RegExp in the CJS spec.

If "no", to de-confuse about the module name, please add a hint that another spec was used (and if you know which, which), and that results can thus be incompatible with the CJS UT spec.

ScottFreeCode commented 7 years ago

As a random bystander, I don't see what's ambiguous about "derived from" in the readme -- in my experience that normally means "based on but with changes" or "a modified copy of", whereas if this was intended to implement or adhere to the spec it would say, well, "implementation of" or "adheres to" (by extension, since it doesn't say so, I would not expect it to implement or adhere to the spec).

Besides which, most assertion libraries as far as I've ever heard don't aim to implement any spec at all (though many aim to imitate each other), so it's not necessarily really implementing some other spec and failing to list that other spec.

mk-pmb commented 7 years ago

by extension, since it doesn't say so, I would not expect it to implement or adhere to the spec

Yeah, that's my assumption now; however, would you assume conformity from the current module name?

ScottFreeCode commented 7 years ago

Hmm... The name at the top of the readme is just "Assert", which fits with how I found this looking for the browser-compatible implementation that Webpack and Browserify use in place of Node's built-in assert module. The GitHub repository name I sort of assumed meant it's an assertion lib for CommonJS (as opposed to AMD compatibility I suppose? or because it was based on Node, which is supposed to be a mostly CommonJS environment?)... but I guess if you focus on the GitHub repository name and the reference to the CommonJS UT spec they seem more suggestive of a relationship than either the name or the reference would on its own. So I can see where that could probably be a bit clearer; but on the other hand, I don't know that it's likely to be the thing developers are expected to put together (as opposed to, say, "the name at the top is Assert and it has something to do with the assert module in Node").

No strong opinions from me, then. 😉

mk-pmb commented 7 years ago

Thanks for pointing it out! I think I got something mixed up at the other day's NPM search. Indeed, even on NPM the package name is just assert, and "commonjs assert" is just in the start of the description. So the easy way out would be to change the description [1] and rename this repo. When I rename repos, Github usually installs a redirect to the new name.

[1] e.g. "An assert module compatible with node.js and browsers, inspired by but better than the CommonJS Unit Testing spec"

ScottFreeCode commented 7 years ago

:+1: on "better than"; the fact that 42 == [42] is true in JavaScript is confusing enough without getting the same oddity in deepEqual (which isn't strict, but is supposed to match structures)!

calvinmetcalf commented 7 years ago

I am more than happy to accept a pull request updating the readme to emphasize that it is a browser implementation of the node.js assert library which is based on the commonjs assert spec