Skip to content
This repository has been archived by the owner on Jan 6, 2021. It is now read-only.

Format symbols #26

Merged
merged 6 commits into from Mar 1, 2019
Merged

Format symbols #26

merged 6 commits into from Mar 1, 2019

Conversation

BrandonE
Copy link
Contributor

@BrandonE BrandonE commented Mar 1, 2019

Purpose (TL;DR) - mandatory

Properly format JavaScript symbols. Related to sinon#1974.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. npm test. This PR adds a symbolic key to one of the objects in a unit test and verifies that it is rendered properly.

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@coveralls
Copy link

coveralls commented Mar 1, 2019

Pull Request Test Coverage Report for Build 103

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

Files with Coverage Reduction New Missed Lines %
formatio.js 9 82.46%
Totals Coverage Status
Change from base Build 100: 0.5%
Covered Lines: 94
Relevant Lines: 112

💛 - Coveralls

Copy link
Member

@mantoni mantoni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👍

@mantoni mantoni merged commit ff37cfd into sinonjs:master Mar 1, 2019
@mantoni
Copy link
Member

mantoni commented Mar 1, 2019

Released in v.3.2.0.

@BrandonE BrandonE deleted the symbols branch March 1, 2019 07:37
@mroderick
Copy link
Member

Very nice!

@bhousel
Copy link

bhousel commented Mar 2, 2019

Ahh it looks like this change is triggering failures on engines that don't support Object.getOwnPropertySymbols

I noticed this because our project uses PhantomJS for our tests, but IE11 support would also be an issue.
We can polyfill this as a workaround, but thought you'd want to know!
openstreetmap/iD#6001

@mantoni
Copy link
Member

mantoni commented Mar 2, 2019

Thank you for letting us know @bhousel! We will run into the same issue with the Sinon test suite. I didn't think of IE 11 when reviewing. I'll prepare a patch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants