Refactoring es2016nify and HashStream replace createSha1Hash()#35
Refactoring es2016nify and HashStream replace createSha1Hash()#35segayuu wants to merge 25 commits intohexojs:masterfrom segayuu:refactoring-from-lebab
Conversation
- __defineGetter/Setter__ to Object.defineProperty()
|
travis ci is causing an error is an error specific to |
|
maybe issue gotwarlost/istanbul#717 ... |
lib/highlight.js
Outdated
| }); | ||
|
|
||
| result += '<figure class="highlight' + (data.language ? ' ' + data.language : '') + '">'; | ||
| let result = `<figure class="highlight${language ? ` ${language}` : ''}">`; |
There was a problem hiding this comment.
In the future maybe we can go into some of these ternary expressions and make them more explicit. That was there before so I didn't want to block it for this PR.
There was a problem hiding this comment.
@tcrowe thank review!
As you can see, I set it explicitly.
JLHwung
left a comment
There was a problem hiding this comment.
LGTM as the new coverage missing line is of integration with hightlight.js
|
@JLHwung |
|
This will bring ...
it('HashStream', () => {
const content1 = '123456';
const content2 = '654321';
const stream = new hash.HashStream();
// explicit convert
stream.write(Buffer.from(content1));
// implicit convert
stream.write(content2);
stream.end();
stream.read().should.eql(sha1(content1 + content2));
});
... |
lib/cache_stream.js
Outdated
|
|
||
| CacheStream.prototype._transform = function(chunk, enc, callback) { | ||
| var buf = chunk instanceof Buffer ? chunk : Buffer.from(chunk, enc); | ||
| const buf = chunk instanceof Buffer ? chunk : Buffer.from(chunk, enc); |
There was a problem hiding this comment.
I can't get this Buffer.from branch to run in the test. Can it ever be something other than Buffer? If not the Buffer.from branch can be removed and coverage will be 100%.
There was a problem hiding this comment.
Or you can do something tricky like this in ./test/cache_stream.js
it('default', () => {
const src = new Readable();
const cacheStream = new CacheStream();
const content1 = Buffer.from('test1');
const content2 = 'test2';
let transformed = false;
src.push(content1);
src.push(null);
src.pipe(cacheStream);
cacheStream._transform(content2, 'utf8', () => { transformed = true; });
cacheStream.on('finish', () => {
cacheStream.getCache().should.eql(content1 + content2);
transformed.should.be.true;
});
});There was a problem hiding this comment.
https://nodejs.org/api/stream.html#stream_transform_transform_chunk_encoding_callback
Even if stream is not an objectMode, it is possible to pass string. For this reason, we need to assume that string will flow.
There was a problem hiding this comment.
The type of chank of _transform()...
- Set
decodeStringsof theTransform (super: Writeble)class's constructor option tofalse. - Calling
setEncoding()on the sourceReadable.
Only when it meets the above conditions, it is string, otherwise it is Buffer.
There was a problem hiding this comment.
After finishing the previous behavior check, in the case of the current implementation, we were able to grasp that chunk is only Buffer ...
|
@segayuu It's looking good. Whenever you feel like it's finished let me know and I'll test, review, & merge. |
|
@tcrowe thank! https://github.com/segayuu/hexo-util/blob/refactoring-from-lebab/lib/hash.js#L19 For this reason, I am worried about whether to implement it so that it can deal with object or worry about deleting implementation other than algorithm as a useless wrapper class. |
|
Only What do you think we should do? |
|
For me, the idea is to make the current |
|
@segayuu Okay cool! 🤓 |
- deprecated HashStream() - HashStream() to no-ObjectMode stream
|
@tcrowe It would be greatly appreciated if you could do a second review. |
|
@segayuu Okay! It might be when I wake up in the morning. It's late here now. |
|
I waited for January, but I would like to split PR. |
No description provided.