FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix default encoding of LazyTransform [2] by zvictor · Pull Request #8624 · nodejs/node · GitHub

/ node Public

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

Closed
zvictor wants to merge 3 commits into
nodejs:masterfrom
zvictor:lazy-transform
ConversationCommits (3)ChecksFiles changed
Closed

crypto: fix default encoding of LazyTransform [2]#8624
zvictor wants to merge 3 commits into
nodejs:masterfrom
zvictor:lazy-transform

Conversation

zvictor commented Sep 17, 2016 •
edited
Loading

Copy link
Copy Markdown
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.

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 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
crypto: fix default encoding of LazyTransform
41d63aa 
Change the default encoding from latin1 to utf8
and extend the test-crypto tests.
crypto: add new tests for the lazy-transform
e689066
yorkie reviewed Sep 17, 2016
View reviewed changes
Comment thread lib/internal/crypto/lazy_transform.js

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

yorkie Sep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed Sep 17, 2016
View reviewed changes
Comment thread test/parallel/test-crypto.js
crypto.createHash('sha1').update({foo: 'bar'});
}, /buffer/);


yorkie Sep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This change seems not necessary :)

yorkie commented Sep 17, 2016

Copy link
Copy Markdown
Contributor

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

zvictor commented Sep 17, 2016

Copy link
Copy Markdown
Author

@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 added the blocked PRs that are blocked by other issues or PRs. label Sep 17, 2016

addaleax commented Sep 17, 2016

Copy link
Copy Markdown
Member

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

mscdex removed the build Issues and PRs related to build files or the CI. label Sep 17, 2016
Fishrock123 reviewed Sep 17, 2016
View reviewed changes
Comment thread lib/crypto.js
const stream = require('stream');
const util = require('util');
const LazyTransform = require('internal/streams/lazy_transform');
const LazyTransform = require('internal/crypto/lazy_transform');

Fishrock123 Sep 17, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

jasnell Sep 20, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 20, 2016

Copy link
Copy Markdown
Member

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 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 commented Sep 21, 2016

Copy link
Copy Markdown
Author

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 commented Sep 21, 2016

Copy link
Copy Markdown
Member

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 notifications@github.com 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 force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

fhinkel commented Jun 7, 2017

Copy link
Copy Markdown
Contributor

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 closed this Jun 7, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
+2 more reviewers
Fishrock123 Fishrock123 left review comments
yorkie yorkie left review comments
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL