Replace isbinaryfile with istextorbinary#738
Conversation
lib/actions/actions.js
Outdated
There was a problem hiding this comment.
Let's not change value of a variable inside the if statement. It's making it harder to reason about.
isText = istextorbinary.isTextSync(undefined, body)
if (isText) {
// foo
}
// etcThere was a problem hiding this comment.
well, it doesn't change any value, but i'll extract it np.
There was a problem hiding this comment.
It changes var binary value. You should get rid of this variable.
It is easier to think about a thing that doesn't change than mutable variable values.
Toilal
force-pushed
the
feat/istextorbinary
branch
from
faa963b to
541c3dc
Compare
Toilal
force-pushed
the
feat/istextorbinary
branch
from
541c3dc to
bef92f8
Compare
There was a problem hiding this comment.
It's not really part of this PR, but why this is not called for binary data ?
There was a problem hiding this comment.
That's mostly legacy feature. You'll have the capacity to edit binary content using the new file system (package is mem-fs-editor).
Replace isbinaryfile with istextorbinary
|
LGTM thanks for taking care of it! |
|
A pleasure, great to have responsive people like your team in this project :) |
3 participants
This avoid reading the file twice, and also drop one dependency when #737 is merged.
compared to isbinaryfile, istextorbinary can handles a
Bufferas input instead of filename.