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

Support js-yaml options #1

Merged
merged 3 commits into from Dec 5, 2014

Conversation

Projects
None yet
2 participants
@shinnn
Copy link
Collaborator

commented Dec 5, 2014

  • I updated all the tests to work fine.
  • I updated the main script to support js-yaml options.
  • I updated all the dependencies.
  • I removed unnecessary bower.json.
  • I updated .jshintrc.
    • Now JSHint supports mocha option so we don't have to pre-define global variables of Mocha explicitly.

Before merging this PR, please enable Travis CI build.

@jonschlinkert

This comment has been minimized.

Copy link
Owner

commented Dec 5, 2014

wow, thanks! I appreciate it!

jonschlinkert added a commit that referenced this pull request Dec 5, 2014

Merge pull request #1 from shinnn/master
Support js-yaml options

@jonschlinkert jonschlinkert merged commit 9da7fc5 into jonschlinkert:master Dec 5, 2014

@shinnn

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 5, 2014

I'd happily maintain this repo if you want to collab me :)

@jonschlinkert

This comment has been minimized.

Copy link
Owner

commented Dec 5, 2014

done!

@shinnn

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 5, 2014

Great! Thanks.

};

function createYamlOptions (options, filepath) {
if (typeof options === 'string') {
return {filename: filepath};

This comment has been minimized.

Copy link
@jonschlinkert

jonschlinkert Dec 5, 2014

Owner

@shinnn since js-yaml uses this for error reporting and filepath should always be a valid path (I think, unless we're trying to parse strings too), might make sense to always pass this on the options. e.g.

module.exports.sync = function (filepath, options) {
  options = xtend({filename: filepath}, options);
  var str = fs.readFileSync(filepath, options.enc || 'utf8');
  return YAML.safeLoad(str, options);
};

Or is there another goal here?

This comment has been minimized.

Copy link
@shinnn

shinnn Dec 5, 2014

Author Collaborator

Why I checked the type (typeof options === 'string') here is that I want to support the case when a user passes the encoding string directly to the second argument like fs.readFile.

readYamlSync('fixture.yml', 'base64');

I know it's the very, very edge case. Almost all the YAML file is encoded in UTF-8.
It's OK to drop encoding string support.

Even if we drop it, we can do the same thing as below:

readYamlSync('fixture.yml', {encoding: 'base64'});

This comment has been minimized.

Copy link
@jonschlinkert

jonschlinkert Dec 5, 2014

Owner

that's what I thought, that's fine. No need to change unless you want to. thanks for clarifying

This comment has been minimized.

Copy link
@jonschlinkert

jonschlinkert Dec 5, 2014

Owner

actually, can js-yaml parse anything besides a utf8 encoded string? If that's the case let's just hard-code to utf8. But if js-yaml can parse other encodings, what if a user wants to pass options to js-yaml and set the encoding at the same time?

This comment has been minimized.

Copy link
@shinnn

shinnn Dec 5, 2014

Author Collaborator

I'll show you an example using read-yaml-promise which I created a little while ago.

  1. The target file is encoded in UCS-2. (We can read the content on browser but don't care about it. Github seems to encode non-utf-8 files to utf-8 as possible for browsing.)
  2. Read the file with UCS-2 encoding. The function get an utf-8-encoded YAML string.
  3. Inside the function, js-yaml parses the utf-8-encoded string.

This comment has been minimized.

Copy link
@shinnn

shinnn Dec 5, 2014

Author Collaborator

If that's the case let's just hard-code to utf8.

I think it's not needed because js-yaml itself encodes the string in utf-8 before parsing.
https://github.com/nodeca/js-yaml/blob/a6cd5657603496c7128d3b9fa621ce9274283194/lib/js-yaml/loader.js#L1506

This comment has been minimized.

Copy link
@jonschlinkert

jonschlinkert Dec 5, 2014

Owner

The target file is encoded in UCS-2. ... I think it's not needed because js-yaml itself encodes the string

very cool, thanks for teaching me that. good to know.

@jonschlinkert

This comment has been minimized.

Copy link
Owner

commented Dec 5, 2014

I cleaned up some things, feel free to change this however you want. As long as you follow semver I'll be happy :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.