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

Support for ImageData instantiation using a source array #45

Merged
merged 3 commits into from
Jul 31, 2019

Conversation

lualparedes
Copy link
Contributor

This PR introduces changes in the ImageData class to comply with the official specification.

Changes

  • ImageData() now supports instantiation using the syntax: new ImageData(array, width [, height]);.
  • More semantic error types.
  • Test suite for the class was refactored, each inner describe block corresponds to each valid way to instantiate ImageData. All tests from the previous version are in the first inner describe block.

@coveralls
Copy link

coveralls commented Jul 25, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 44a1df8 on lualparedes:master into 7d6239c on hustcc:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 99.931% when pulling 27624eb on lualparedes:master into 7d6239c on hustcc:master.

@jtenner
Copy link
Collaborator

jtenner commented Jul 25, 2019

Wow. At first glance, this looks great and comprehensive. The test suite meets my expectations, and it appears to follow the specification.

I will take some time to read the specs and go over the algorithms to look for some improvements, and inspect why coverage is reported at 99.931%. (Very strange.)

@lualparedes
Copy link
Contributor Author

Hi @jtenner, thanks for replying quickly.

The coverage issue was caused by a single line that wasn't covered because of a small typo in the test suite. The issue was fixed in the second commit.

@hustcc
Copy link
Owner

hustcc commented Jul 26, 2019

@jtenner You can decide whether to merge the PR or not. And what is your npm id, you can help to release the new version.

@jtenner
Copy link
Collaborator

jtenner commented Jul 26, 2019

Thank you @hustcc.

I really like this pull request. Just want to give myself a little time with it to make sure everything is truly covered.

The honest truth is it was a total oversight of mine not to have included it in the first place :)

@jtenner
Copy link
Collaborator

jtenner commented Jul 31, 2019

Alright! Looks good. I'm pretty confident this will be okay in production. It's just that this software was downloaded 88k times this week, and I wanted to sit on the pull request until I was absolutely confident it would be okay.

@hustcc we can release a minor update 2.2.0.

Is it possible to get access to publish this on npm as well?

npm id: jtenner

@jtenner jtenner merged commit 29374b5 into hustcc:master Jul 31, 2019
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.

4 participants