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

Rewrite pretty-printing HTML elements to prevent throwing internal errors #275

Merged
merged 1 commit into from Aug 9, 2014

Conversation

@DrRataplan
Copy link
Contributor

@DrRataplan DrRataplan commented Jun 20, 2014

when using non-native DOM implementations.

fixes #274

@logicalparadox
Copy link
Member

@logicalparadox logicalparadox commented Jun 21, 2014

My initial review of #274 and your change seems to make sense, however....

It looks like you made your change directly to ./chai.js, which is the browser distribution automatically built from the internal source files. Consequently, it means that the CI build was not based on your change. Also, this file is built automatically when you run the test suite locally so it is pretty obvious this is an untested change.

Please make your change to the appropriate source file within ./lib so that it persists. Also, please review our code style guidelines (link). Specifically, your comments exceed the max line width of 80 characters and your tabbing is inconsistent with the rest of the document. Finally, your PR should NOT include ./chai.js; changes to that file are only committed on a version release.

Resource: contribution guide.

…rors

Fixes errors occuring when using a non-native DOM implementation
@DrRataplan
Copy link
Contributor Author

@DrRataplan DrRataplan commented Jun 30, 2014

My apologies, I have now submitted a new commit, which does follow the coding style guidelines.

The fallback for objects which do somehow show up as a false positive for isDomElement are rendered as normal object, meaning their keys are listed.

I do hope this is up to standards.

@DrRataplan
Copy link
Contributor Author

@DrRataplan DrRataplan commented Jul 10, 2014

Any news on merging this pull request?

@copenhas
Copy link

@copenhas copenhas commented Jul 30, 2014

I was having the same issue with chai blowing up trying to format a non-native XML DOM object. I applied the patch locally and it works for me.

+1

logicalparadox added a commit that referenced this pull request Aug 9, 2014
Rewrite pretty-printing HTML elements to prevent throwing internal errors
@logicalparadox logicalparadox merged commit 6c789b1 into chaijs:master Aug 9, 2014
1 check passed
1 check passed
@logicalparadox
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants