Skip to content
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

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

Closed
mk-pmb opened this issue Oct 12, 2016 · 6 comments

Comments

@mk-pmb
Copy link
Contributor

mk-pmb commented Oct 12, 2016

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
Copy link

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
Copy link
Contributor Author

mk-pmb commented Oct 12, 2016

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
Copy link

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
Copy link
Contributor Author

mk-pmb commented Oct 12, 2016

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
Copy link

👍 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
Copy link
Collaborator

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

mk-pmb added a commit to mk-pmb/nodejs-assert-for-browsers that referenced this issue Oct 13, 2016
Kept the package version in case you prefer to increase it via npm,
and/or rename the repo before next release.

Fixes browserify#27.
mk-pmb added a commit to mk-pmb/nodejs-assert-for-browsers that referenced this issue Oct 13, 2016
Kept the package version in case you prefer to increase it via npm,
and/or rename the repo before next release.

Fixes browserify#27.
mk-pmb added a commit to mk-pmb/nodejs-assert-for-browsers that referenced this issue Oct 13, 2016
Kept the package version in case you prefer to increase it via npm,
and/or rename the repo before next release.

Fixes browserify#27.
mk-pmb added a commit to mk-pmb/nodejs-assert-for-browsers that referenced this issue Oct 13, 2016
Kept the package version in case you prefer to increase it via npm,
and/or rename the repo before next release.

Fixes browserify#27.
calvinmetcalf pushed a commit that referenced this issue Oct 26, 2016
Kept the package version in case you prefer to increase it via npm,
and/or rename the repo before next release.

Fixes #27.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants