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.

Sign up for GitHub

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

Jump to bottom

v1.x: Explicitly set default encoding in createHash() #6

Merged
merged 1 commit into from
Feb 23, 2017
Merged

v1.x: Explicitly set default encoding in createHash() #6

merged 1 commit into from
Feb 23, 2017

Conversation

addaleax
Copy link
Contributor

Backport PR for #5 (which does cherry-pick cleanly, btw)

@addaleax
Explicitly set default encoding in createHash()
1b94a51 
Up until now, the default encoding for this is `binary`, but
this is scheduled to change in Node 8. Since this module is
currently always passing string input to the `Hash` object,
it needs to account for that.

A better fix would likely involve dropping all strings handling
and treating binary data using Buffer objects, but I kept
this change minimal to avoid any breakage.

Refs: nodejs/node#8611
@malept malept self-requested a review February 21, 2017 22:34
@malept malept merged commit a30977d into malept:v1 Feb 23, 2017
@addaleax addaleax deleted the fix-default-encoding-v1 branch February 23, 2017 14:48
@addaleax
Copy link
Contributor Author

Awesome, thanks!

@addaleax
Copy link
Contributor Author

@malept Hate to nag, but is there any chance this could go out in a release (and/or be picked up by electron-prebuilt in some other way) in the next couple of days?

@malept
Copy link
Owner

malept commented Feb 25, 2017

I haven't had access to the computer where I release sumchecker lately. I will try within the next couple of days.

@addaleax
Copy link
Contributor Author

addaleax commented Mar 6, 2017

bump @malept

And by the way, according to the sha*sum source code, the binary/text switch doesn’t refer to encodings but to whether to open the file in text mode or not (for e.g. Windows where text mode automatically translates between \r\n and \n). Node doesn’t actually support text mode at all; if it’s something that sumchecker should support, it needs to do that translation itself.

@malept
Copy link
Owner

malept commented Mar 11, 2017

Published 1.3.1 and 2.0.2. Sorry for the delay.

@addaleax
Copy link
Contributor Author

@malept No problem! Thanks for maintaining awesome open source software! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malept malept Awaiting requested review from malept

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@addaleax @malept