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

deepEqual(true, [ 1 ]) #26

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

deepEqual(true, [ 1 ]) #26

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

Comments

@mk-pmb
Copy link
Contributor

mk-pmb commented Oct 12, 2016

Hi!
I'm developing an assert lib and it disagrees with this one:

~/commonjs-assert$ git-log-concise -n 1
Mon 2016-06-27 02:58    f8975ca Add deepStrictEqual to the README (#23)

~/commonjs-assert$ nodejs -e 'require("./assert.js").deepEqual(true, [ 1 ]);'
[…]
AssertionError: true deepEqual [ 1 ]

I'd prefer my lib to also throw an error on this, but I can't find permission for that in the CJS Unit Testing 1.0 spec. According to my lib, rule 7.3 applies, because one of the values is not an object, so they don't "both pass" the typeof object test. As said, I prefer your module's behavior, so what reading of the spec did you come up with to legitimate throwing?

Node.js also throws, but to me it seems it's unintentional there.

@calvinmetcalf
Copy link
Collaborator

this is the same as the node one so we did whatever they did

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Oct 12, 2016

I see, I probably didn't read the Readme proper enought the first time.

The API is derived from the commonjs 1.0

Well, "derived" is a weak term indeed, so do you even claim that this module conforms to the CJS spec?
If not, the module name and line 45 of assert.js would be quite confusing.

@calvinmetcalf
Copy link
Collaborator

This is literally the node code pulled out and slightly modified to work in
a browser

On Wed, Oct 12, 2016, 3:29 PM M.K. notifications@github.com wrote:

I see, I probably didn't read the Readme proper enought the first time.

The API is derived from the commonjs 1.0

Well, derived is a weak term indeed, so do you even claim that this module
conforms to the CJS spec?
If not, the module name and line 45 of assert.js
https://github.com/defunctzombie/commonjs-assert/blob/master/assert.js#L45
would be quite confusing.


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#26 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABE4nw5iX2Q90KGQ9t7iib4JFAomDphbks5qzTUAgaJpZM4KVF-h
.

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Oct 12, 2016

Yeah, that part I understood, thus my question is now about mission and intent. Only when I understand the goals of this module, can I decide which of two possible features I'd like to request.
Do I understand correctly that

modified to work in a browser

was the only intent for this package, and it is not commonjs-assert's objective to be an assert module according to the CommonJS Unit Testing spec?

Edit: Updated the title to better represent the current state of the issue.
Edit 2: Actually, I can split that one even now, since I'd want the answer in the readme anyway.

@mk-pmb mk-pmb changed the title deepEqual(true, [ 1 ]) Does commonjs-assert aim to be an assert module as per CommonJS UT spec? Oct 12, 2016
@mk-pmb mk-pmb changed the title Does commonjs-assert aim to be an assert module as per CommonJS UT spec? deepEqual(true, [ 1 ]) Oct 12, 2016
@ScottFreeCode
Copy link

For what it's worth, I'd rather like to see the relationship of this library to the Node internal module made more explicit in the readme and leave the matter of intention and spec compliance to the Node documentation (but, perhaps link that documentation here).

@mk-pmb
Copy link
Contributor Author

mk-pmb commented Oct 26, 2016

Issue became irrelevant because we clarified that this assert doesn't aim to conform to the CommonJS spec.

@mk-pmb mk-pmb closed this as completed Oct 26, 2016
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