[bz4404935] Added support for using YAML as an alternative config format #580

Merged
merged 23 commits into from Oct 31, 2012

4 participants

@ricallinson

This pull request provides support for using YAML as well as or in place of JSON. The benefit of YAML over JSON is in the ability to add comments in to the configuration.

@ricallinson

I've been trying to get the tests passing for this pr but it seems the functional tests are unstable and throw different errors on each run. All unit-tests pass and the randomly failing functional tests are not connected to the changes in this pr.

@redonkulus redonkulus and 1 other commented on an outdated diff Sep 30, 2012
lib/app/addons/rs/config.server.js
@@ -142,8 +177,8 @@ YUI.add('addon-rs-config', function(Y, NAME) {
if ('.' !== fs.subDir) {
return;
}
- // we only care about json files
- if ('.json' !== fs.ext) {
+ // we only care about json or yaml files
+ if ('.json' !== fs.ext && '.yaml' !== fs.ext) {

Do you also need a check for .yml?

Good catch, added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@redonkulus redonkulus and 1 other commented on an outdated diff Sep 30, 2012
lib/store.server.js
@@ -979,9 +978,11 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
if (CONVENTION_SUBDIR_TYPE_IS_JS[type] && '.js' !== fs.ext) {
return false;
}
- if ('spec' === type && '.json' !== fs.ext) {
+
+ if ('spec' === type && ('.json' !== fs.ext && '.yaml' !== fs.ext)) {

Do you also need a check for .yml?

Good catch, added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish drewfish and 1 other commented on an outdated diff Oct 1, 2012
lib/app/addons/rs/config.server.js
@@ -25,7 +25,8 @@ YUI.add('addon-rs-config', function(Y, NAME) {
var libfs = require('fs'),
libpath = require('path'),
existsSync = libfs.existsSync || libpath.existsSync,
- libycb = require('ycb');
+ libycb = require('ycb'),
+ yaml = require('js-yaml');
@drewfish
Yahoo Inc. member
drewfish added a note Oct 1, 2012

A small matter of style: we sorta have an convention of naming the variable containing the libraries as libfoo. It might be worthwhile to maintain that for consistency.

@isao
isao added a note Oct 1, 2012

I would prefer to drop the "lib" prefix convention personally. It essentially identifies things originating from a require, and I don't see much value in that on node.js.

@drewfish
Yahoo Inc. member
drewfish added a note Oct 1, 2012

One thing that has always been a problem is path = require('path');. In this case, it's really hard to use path as, say, the path of something. So it's just a matter of namespacing -- somehow having the libraries available but not having them chew up useful variable names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish drewfish commented on an outdated diff Oct 1, 2012
lib/app/addons/rs/config.server.js
/**
* Reads and parses a JSON file.
- * @method readConfigJSON
+ * @method readConfigThroughCacheSync
* @param {string} fullPath path to JSON file
@drewfish
Yahoo Inc. member
drewfish added a note Oct 1, 2012

IMHO this is a very clunky function name. Perhaps readConfigSimple() (as a counterpart to readConfigYCB()).

Also the description still says Reads and parses a JSON file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish drewfish and 1 other commented on an outdated diff Oct 1, 2012
lib/app/addons/rs/config.server.js
return Y.mojito.util.copy(json);
},
+ /*
+ This function will try and read the requested file returning an Object.
+
+ It will also trim ".ext" from the filePath
+ */
+
+ readConfigSync: function (filePath, type) {
@drewfish
Yahoo Inc. member
drewfish added a note Oct 1, 2012

Missing yuidoc comments.

Can't the type parameter be inferred from the file extension?

Good catch, will update docs.

The type parameter is used in the recursive call so the function knows which format to test for next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish
Yahoo Inc. member

Need unit test for the new readConfigSync() method. Also need unit test for reading YAML files.

@ricallinson

Updated pr;

  • libyaml used to maintain consistency
  • readConfigThroughCacheSync renamed to readConfigSimple and description updated
  • yuidoc comments added for readConfigSync
  • unit-tests added for readConfigSync
@rwaldura

please also close the bz ticket after merging, thanks!

@drewfish drewfish commented on the diff Oct 9, 2012
lib/app/addons/rs/config.server.js
+ /*
+ This function will try and read the requested file returning an Object.
+
+ It will also trim ".ext" from the filePath
+ */
+
+ /**
+ * Reads and parses a JSON or YAML structured file.
+ * @method readConfigSync
+ * @param {string} fullPath path to JSON or YAML file
+ * @param {int} type is the index of the file-type to read 0: .yml, 1: .yaml, 2: .json
+ * @return {user-defined} contents of file as an object
+ */
+ readConfigSync: function (filePath, type) {
+
+ var file = ['.yml', '.yaml', '.json'],
@drewfish
Yahoo Inc. member
drewfish added a note Oct 9, 2012

Why do we need to attempt the three extensions? Can't we just use libpath.extname(filePath)?

Not sure I understand. How would that help?

Never mind, I get it. The code was originally designed to allow overriding of the ".json" files which is why it uses the indexed array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@drewfish drewfish commented on the diff Oct 9, 2012
package.json
@@ -28,7 +28,8 @@
"ycb": "~1.0.0",
"yui": "~3.5.1",
"yuidocjs": "~0.3.14",
- "yuitest": "~0.7.4"
+ "yuitest": "~0.7.4",
+ "js-yaml": "1.0.2"
@drewfish
Yahoo Inc. member
drewfish added a note Oct 9, 2012

Small small point, but so far the "dependencies" have been sorted alphabetically. Might be nice to maintain that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rwaldura

To recap, goals for this feature:
1- preserve backwards compatibility with all existing config
-> i.e. parse application.json and other config files w/o change
2- allow comments in configuration files
-> i.e. parse app.{yml,yaml}
3- prefer yaml to json
-> warn if both application.json and application.yaml are present, use application.yaml

@lzhan

Just tested "prefer yaml to json": it is true for defaults, definition, routes config files, but not for application. When both application.json application.yaml exist, application.json is used.

@lzhan lzhan merged commit 57eb6b9 into yahoo:develop Oct 31, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment