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

Issue #1936: Make the spy functions non enumerable so that printing it is more concise #1941

Merged
merged 1 commit into from Oct 31, 2018

Conversation

LouisBrunner
Copy link
Contributor

Purpose

Improve issue #1936: Make the spy functions non enumerable so that printing it is more concise

Background

When printing a spy (which is often done by assertion libraries), the output takes a long chunk of space and make debugging really difficult. This PR removes the internal functions from this output, making the list smaller and more useful.

Solution

This PR expands the extend function to support extending using Object.defineProperty with enumerable: false. As configurable and writable are set to true, the rest of the codebase will still work as intended. Note that as indicated by @ehmicky in the original issue, the node REPL is not affected.

How to verify

screen shot 2018-10-28 at 18 38 51

@coveralls
Copy link

coveralls commented Oct 28, 2018

Pull Request Test Coverage Report for Build 2740

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.01%) to 95.1%

Files with Coverage Reduction New Missed Lines %
sinon/util/core/extend.js 1 89.8%
Totals Coverage Status
Change from base Build 2739: 0.01%
Covered Lines: 2051
Relevant Lines: 2118

💛 - Coveralls

@mroderick
Copy link
Member

Thank you for your pull request!

I am surprised that the valid-jsdoc rule didn't complain about this already, or I would have fixed it when I upgraded eslint-config-sinon.

Since you're touching these files anyway, would you mind turning the comments into JSDoc, so we can use them for future purposes?

@mroderick
Copy link
Member

I re-ran the build in Circle CI, I guess it had a hiccup :)

@LouisBrunner
Copy link
Contributor Author

I am not too familiar with JSDoc, are the comments fine like this?

I tested a bit and eslint's valid-jsdoc seems to only check comments that start with /**.

@mroderick
Copy link
Member

I am not too familiar with JSDoc, are the comments fine like this?

If ESLint doesn't complain, then I guess they're good :)

I think that JSDoc comments are supposed to start with /**, which is why it didn't complain about the previous comments.

Thanks!

@mroderick mroderick merged commit b6930c5 into sinonjs:master Oct 31, 2018
@mroderick
Copy link
Member

This has been published as sinon@7.1.1

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

Successfully merging this pull request may close these issues.

None yet

3 participants