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

crypto: fix default encoding of LazyTransform [2] #8624

Closed
wants to merge 3 commits into from
Closed

crypto: fix default encoding of LazyTransform [2] #8624

wants to merge 3 commits into from

Conversation

zvictor
Copy link

@zvictor zvictor commented Sep 17, 2016 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

Description of change

depends on #8611.

  • move lazy_transform out of lib/internals/streams as it is not used by streams.
  • improve lazy_transform tests and move them to an independent file.

@zvictor
crypto: move lazy_transform.js from /lib/internals/stream
82a9afe 
it does not belong to Streams but Crypto
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. crypto Issues and PRs related to the crypto subsystem. labels Sep 17, 2016
@zvictor zvictor changed the title Lazy transform crypto: fix default encoding of LazyTransform [2] Sep 17, 2016
Lukas Möller and others added 2 commits September 17, 2016 14:00
@zvictor
crypto: fix default encoding of LazyTransform
41d63aa 
Change the default encoding from latin1 to utf8
and extend the test-crypto tests.
@zvictor
crypto: add new tests for the lazy-transform
e689066
yorkie
yorkie reviewed Sep 17, 2016
View reviewed changes
lib/internal/crypto/lazy_transform.js

if (!this._options || !this._options.defaultEncoding) {
this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to use defaultEncoding of options in the following way?

if (this._options && this._options.defaultEncoding) {
  this._writableState.defaultEncoding = this._options.defaultEncoding;
} else {
  this._writableState.defaultEncoding = crypto.DEFAULT_ENCODING;
}

yorkie
yorkie reviewed Sep 17, 2016
View reviewed changes
test/parallel/test-crypto.js
@@ -53,7 +53,6 @@ assert.throws(function() {
crypto.createHash('sha1').update({foo: 'bar'});
}, /buffer/);


Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems not necessary :)

@yorkie
Copy link
Contributor

yorkie commented Sep 17, 2016

By the way, this seems duplicated with #8611 :(

@zvictor
Copy link
Author

zvictor commented Sep 17, 2016

@yorkie it depends on it. @lmoe fixed it before me, I just moved the file and tried to improve the tests, as mentioned in the description of this PR.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2016
@addaleax
Copy link
Member

I’m labelling this as blocked then, we can take a look again once #8611 lands :)

@mscdex mscdex removed the build Issues and PRs related to build files or the CI. label Sep 17, 2016
Fishrock123
Fishrock123 reviewed Sep 17, 2016
View reviewed changes
lib/crypto.js
@@ -21,7 +21,7 @@ const timingSafeEqual = binding.timingSafeEqual;
const Buffer = require('buffer').Buffer;
const stream = require('stream');
const util = require('util');
const LazyTransform = require('internal/streams/lazy_transform');
const LazyTransform = require('internal/crypto/lazy_transform');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file should probably still be in /streams?

Copy link
Member

Choose a reason for hiding this comment

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

The lazy_transform internal class is only used by the crypto module. I discussed this with @zvictor at the code and learn this last weekend and we went through and made sure. I'm happy with moving it in order to make the relationship between this class and the crypto module clear.

@jasnell
Copy link
Member

jasnell commented Sep 20, 2016

We knew there would be some duplicates coming out of the code and learn so please bear with us @zvictor! I really appreciate you taking the time to make these changes. If this particular PR doesn't land because of the duplicate, then there are still plenty of other areas to jump in! :-)

@jasnell jasnell added crypto Issues and PRs related to the crypto subsystem. and removed crypto Issues and PRs related to the crypto subsystem. labels Sep 20, 2016
@zvictor
Copy link
Author

zvictor commented Sep 21, 2016

No problem @jasnell, it was a nice experience anyway.
After I saw the issue had already been fixed I focused on improving the tests and I hope they are useful :)

@jasnell
Copy link
Member

jasnell commented Sep 21, 2016

It's very useful and very appreciated! I hope you stick around and find
other ways of contributing also!

On Wednesday, September 21, 2016, zVictor [email protected] wrote:

No problem @jasnell https://github.com/jasnell, it was a nice
experience anyway.
After I saw the issue had already been fixed I focused on improving the
tests and I hope they are useful :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8624 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eRCSB-Vv9YYbRYtVlmhBvF1seHEoks5qsSMmgaJpZM4J_lFg
.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@fhinkel
Copy link
Member

fhinkel commented Jun 7, 2017

I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed.

@fhinkel fhinkel closed this Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

@Fishrock123 Fishrock123 Fishrock123 left review comments

@yorkie yorkie yorkie left review comments

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. crypto Issues and PRs related to the crypto subsystem. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zvictor @yorkie @addaleax @jasnell @fhinkel @Fishrock123 @mscdex @nodejs-github-bot