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

Expose readFile encoding option through vfs.src #66

Closed
wants to merge 2 commits into from
Closed

Expose readFile encoding option through vfs.src #66

wants to merge 2 commits into from

Conversation

limdauto
Copy link

Here is a PR for src regarding #23.

I will add support for dest if this is up to par :)

@@ -14,7 +15,9 @@ function getContents(opt) {

// read and pass full contents
if (opt.buffer !== false) {
return bufferFile(file, cb);
// set encoding type if user specifies it
var options = 'encoding' in opt ? {'encoding': opt.encoding} : null;
Copy link
Member

Choose a reason for hiding this comment

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

Why not just var options = { encoding: opt.encoding }? If opt.encoding is undefined, it'll be passed through as undefined, which is fine.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. Fixed.

@callumacrae
Copy link
Member

lgtm except for the one thing

@yocontra
Copy link
Member

Test fail?

@limdauto
Copy link
Author

@contra it's failing upstream

file.contents = stripBom(data);
// if user specifies encoding type, a string is returned
// so we need to convert it back to buffer
if (!Buffer.isBuffer(data) && typeof data === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

just typeof is sufficient here, no need for both checks. then you can make this a ternary

Copy link
Author

Choose a reason for hiding this comment

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

That's true

Copy link
Member

Choose a reason for hiding this comment

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

if (typeof data === 'string') {
  data = new Buffer(data, opt.encoding);
}
file.contents = stripBom(data);

would be cleanest imo

@yocontra
Copy link
Member

How does this work with writing files out? Aren't we going to need to specify the encoding there as well

@limdauto
Copy link
Author

@contra I understand without passing encoding to writing out, all this is kind of pointless. I just made the PR to see if I'm on the right track. Will add dest shortly.

@sindresorhus
Copy link

What use-case is this solving exactly? When would you ever need to be able to specify ascii encoding?

@phated
Copy link
Member

phated commented Jun 30, 2015

Encoding doesn't really make sense because we always read in as buffer, which doesn't have an encoding. Plugins use toString() on that buffer to turn it into a string, so there's not a good way to expose the encoding information without breaking all plugins.

@sindresorhus
Copy link

I would think https://github.com/heldinz/gulp-convert-encoding would be a better solution if you're using obscure encodings.

@limdauto
Copy link
Author

Hi guys, so sorry I completely forgot about this PR. It was more than a month ago and I have indeed found out that it's better to convert my stuff to sane encoding first rather than requiring it from gulp/vinyl-fs. Thanks for your comments. I'm closing this.

@limdauto limdauto closed this Jun 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants