Skip to content

Will Shiv and Anna #21

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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Will Shiv and Anna #21

wants to merge 29 commits into from

Conversation

WillSkelton
Copy link

No description provided.

Shiv Unadkat and others added 29 commits February 16, 2017 15:03
all logic sorted out, we chillin
code reduced with buffer.fill()
inverted bitmap writes to file
black and inverted works
creating new buffers for each transform
done for now. a few simple transforms, will think about more complex …
Copy link
Contributor

@bnates bnates left a comment

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul y'all killed it!!! great job on this assignment!

"impliedStrict": true
},
"extends": "eslint:recommended"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul .eslintrc looks good

# .nfs files are created when an open file is removed but is still being accessed
.nfs*

# End of https://www.gitignore.io/api/node,vim,osx,macos,linux
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul .gitignore looks good

"impliedStrict": true
},
"extends": "eslint:recommended"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul duplicate eslintrc???

# .nfs files are created when an open file is removed but is still being accessed
.nfs*

# End of https://www.gitignore.io/api/node,vim,osx,macos,linux
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul same deal here - duplicate .gitignore???

* ability to handle various sized bitmap
* ability to handle LE and BE computers with a single if statement
* utilizes a command line interface (CLI)
* CLI can select the transforms
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul documentation outlined in your README is excellent - nice job on this

return redBuf;
};

exports.greyScale = function(bitmap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul the transform methods in this file are really good - that said, they may be better served as prototype methods built on to the constructor - the constructor could hold the raw buffer and the methods could then interact directly with them

all that said, this is still really clean and good

if (err) throw err;
});

fs.writeFile(`${__dirname}/../img/grey.bmp`, transforms.greyScale(bitmap), function(err, data) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul too many nested writeFile calls here - if the transforms were part of a series of prototype methods then you could simply chain them

"gulp": "^3.9.1",
"mocha": "^3.2.0"
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul package.json looks as expected

fileReader(`${__dirname}/../img/black.bmp`, function(err, data) {
expect(err).to.equal(null);
// expect(data.toString()).to.be.a('string');
expect(data.toString('utf-8').slice(54, 1078)).to.equal('0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000');
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul hahaha - awesome test

// describe('turnBlack switches palette to black', function() {
// it('')
// });
// });
Copy link
Contributor

Choose a reason for hiding this comment

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

@William-Skelton @shivprogrammer @annaul best to remove unused comment blocks from the master branch - these should be contained in a feature or dev branch

overall, excellent tests - you are testing for appropriate outcomes, which is great - only nitpick would be to extract and cluster these tests into 2 files - this one doesn't follow a really cohesive pattern

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.

3 participants