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

streams: refactor LazyTransform to internal/ #2566

Conversation

brendanashworth
Copy link
Contributor

This commit refactors LazyTransform from the crypto implementation
(lib/crypto.js) into an internal module (not publicy accessible) in
internal/streams/LazyTransform.js. This promotes a more modular core
design and removes code bloat in crypto, as LazyTransform didn't
specifically have anything to do with cryptography, but rather a fast
way to support two APIs on a stream.

This commit refactors LazyTransform from the crypto implementation
(lib/crypto.js) into an internal module (not publicy accessible) in
internal/streams/LazyTransform.js. This promotes a more modular core
design and removes code bloat in crypto, as LazyTransform didn't
specifically have anything to do with cryptography, but rather a fast
way to support two APIs on a stream.
@brendanashworth brendanashworth added crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem. labels Aug 26, 2015
@brendanashworth
Copy link
Contributor Author

r=@nodejs/crypto ?

@indutny
Copy link
Member

indutny commented Sep 11, 2015

I think it would be the first module to use camel case file name. Should it really have it? Otherwise, LGTM

@brendanashworth
Copy link
Contributor Author

I don't have a preference, I can change it to lazy-transform.js or lazy_transform.js if you'd prefer it that way. I'm just used to uppercased for constructor files.

@indutny
Copy link
Member

indutny commented Sep 12, 2015

I think lazy_transform should be the best.

@brendanashworth
Copy link
Contributor Author

Updated with a rename. Here is a CI: https://ci.nodejs.org/view/iojs/job/node-test-pull-request/284/.

@indutny
Copy link
Member

indutny commented Sep 12, 2015

LGTM

@Fishrock123
Copy link
Contributor

Hmm, not sure if CI is happy. New run: https://ci.nodejs.org/job/node-test-pull-request/297/

@Fishrock123
Copy link
Contributor

@brendanashworth CI seems good.

brendanashworth added a commit that referenced this pull request Sep 15, 2015
This commit refactors LazyTransform from the crypto implementation
(lib/crypto.js) into an internal module (not publicy accessible) in
internal/streams/lazy_transform.js. This promotes a more modular core
design and removes code bloat in crypto, as LazyTransform didn't
specifically have anything to do with cryptography, but rather a fast
way to support two APIs on a stream.

PR-URL: #2566
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@brendanashworth
Copy link
Contributor Author

Thank you, landed in c70c778.

brendanashworth added a commit that referenced this pull request Sep 15, 2015
This commit refactors LazyTransform from the crypto implementation
(lib/crypto.js) into an internal module (not publicy accessible) in
internal/streams/lazy_transform.js. This promotes a more modular core
design and removes code bloat in crypto, as LazyTransform didn't
specifically have anything to do with cryptography, but rather a fast
way to support two APIs on a stream.

PR-URL: #2566
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@rvagg rvagg mentioned this pull request Sep 15, 2015
brendanashworth added a commit that referenced this pull request Sep 15, 2015
This commit refactors LazyTransform from the crypto implementation
(lib/crypto.js) into an internal module (not publicy accessible) in
internal/streams/lazy_transform.js. This promotes a more modular core
design and removes code bloat in crypto, as LazyTransform didn't
specifically have anything to do with cryptography, but rather a fast
way to support two APIs on a stream.

PR-URL: #2566
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Sep 15, 2015
7 tasks
@rvagg rvagg mentioned this pull request Sep 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants