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

Fixes for bugs found during review #3

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

Conversation

rlindsgaard
Copy link
Member

Bugs described in #2

@@ -1,7 +1,7 @@
{
"name": "cubehash.js",
"description": "CubeHash is a very simple cryptographic hash function",
"version": "1.0.0",
"version": "1.0.1",
Copy link
Member

Choose a reason for hiding this comment

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

Never make version changes on branches. When merged into master use npm version patch to get the sae functionality, but also have the version bump in a separate commit with a git tag as well

@Munter
Copy link
Member

Munter commented Oct 25, 2015

Hash changes based on a change in the implementation point at a major version increase when this gets merged, since the changes are not backwards compatible

@Munter
Copy link
Member

Munter commented Oct 25, 2015

I'd really like if we could improve the test suite to reflect all of these findings. We're just asserting on the resulting outcome instead of testing the mechanics of the algorithm, thus making the test useless in testing the actual specifications

@rlindsgaard
Copy link
Member Author

It would be nice to have proper tests of the internals. I propose merging this in, bump the version (I don't really care if it is a minor or major version) and then start a develop branch where we start with rewriting to make tests and maybe implement a more general version. The constants used are more or less grabbed from the thin air as it is right now.

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.

2 participants