-
Notifications
You must be signed in to change notification settings - Fork 3
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
Data and File Parsing #6
Changes from 15 commits
95b593a
276c854
5f9b426
361e64f
a8ca785
f9ba02d
184451d
2021246
9f4fc3b
0321b6d
608509a
612131e
f733af8
bd93067
cc5895f
38376cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,12 @@ | ||
import Handlebars from 'handlebars'; | ||
import yaml from 'js-yaml'; | ||
import { merge } from './utils'; | ||
|
||
const defaults = { | ||
data: { | ||
src: ['src/data/**/*.yaml'], | ||
parseFn: (contents, path) => yaml.safeLoad(contents) | ||
}, | ||
templates: { | ||
handlebars: Handlebars, | ||
helpers : {}, | ||
|
@@ -30,8 +35,8 @@ function mergeDefaults (options = {}) { | |
* @return {object} User options | ||
*/ | ||
function translateOptions (options = {}) { | ||
/* eslint-disable prefer-const */ | ||
const { | ||
data, | ||
handlebars, | ||
helpers, | ||
layouts, | ||
|
@@ -48,8 +53,19 @@ function translateOptions (options = {}) { | |
partials | ||
} | ||
}; | ||
// @TODO: Is there are more concise way of handling this? | ||
// If you use the pattern above, an object value for `data` | ||
// will get improperly nested/trounced | ||
if (data) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I acknowledge this is gross, but I am so weary of this stuff at the moment :). |
||
if (typeof data === 'string') { | ||
result.data = { | ||
src: data | ||
}; | ||
} else { | ||
result.data = data; | ||
} | ||
} | ||
return result; | ||
/* eslint-enable prefer-const */ | ||
} | ||
|
||
const parseOptions = options => mergeDefaults(translateOptions(options)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,26 @@ import {readFile as readFileCB} from 'fs'; | |
var readFile = Promise.promisify(readFileCB); | ||
|
||
/* Helper functions */ | ||
const basename = filepath => path.basename(filepath, path.extname(filepath)); | ||
const dirname = filepath => path.normalize(path.dirname(filepath)); | ||
const parentDirname = filepath => dirname(filepath).split(path.sep).pop(); | ||
const removeNumbers = str => str.replace(/^[0-9|\.\-]+/, ''); | ||
const getFiles = glob => globby(glob, {nodir: true }); | ||
|
||
function basename (filepath) { | ||
return path.basename(filepath, path.extname(filepath)); | ||
} | ||
function dirname (filepath) { | ||
return path.normalize(path.dirname(filepath)); | ||
} | ||
function parentDirname (filepath) { | ||
return dirname(filepath).split(path.sep).pop(); | ||
} | ||
function removeLeadingNumbers (str) { | ||
return str.replace(/^[0-9|\.\-]+/, ''); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @erikjung Are you comfortable with a possible breaking change? I think this pattern is better, too, but will it cause unexpected side effects or should we be all "full speed ahead, damn the torpedoes"?! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've pushed some comments/examples on most of those utility functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're using the pattern from Fabricator verbatim, then we can leave this as-is and make an issue to improve later. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The above are refactored to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do appreciate the readability/scannability as top-level functions. :) |
||
/** | ||
* @param {glob} glob | ||
* @return {Promise} resolving to {Array} of files matching glob | ||
*/ | ||
function getFiles (glob) { | ||
return globby(glob, { nodir: true }); | ||
} | ||
|
||
/** | ||
* Utility function to test if a value COULD be a glob. A single string or | ||
|
@@ -28,33 +43,56 @@ function isGlob (candidate) { | |
} | ||
|
||
/** | ||
* Take a glob; read the files. Return a Promise that ultimately resolves | ||
* to an Array of objects: | ||
* [{ path: original filepath, | ||
* contents: utf-8 file contents}...] | ||
* Take a glob; read the files, optionally running a `contentFn` over | ||
* the contents of the file. | ||
* | ||
* @param {glob} glob of files to read | ||
* @param {Object} Options: | ||
* - {Function} contentFn(content, path): optional function to run over content | ||
* in files; defaults to a no-op | ||
* - {String} encoding | ||
* | ||
* @return {Promise} resolving to Array of Objects: | ||
* - {String} path | ||
* - {String || Mixed} contents: contents of file after contentFn | ||
*/ | ||
function readFiles (glob) { | ||
function readFiles (glob, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. readFiles expanded to take optional |
||
contentFn = (content, path) => content, | ||
encoding = 'utf-8' | ||
} = {}) { | ||
return getFiles(glob).then(paths => { | ||
var fileReadPromises = paths.map(path => { | ||
return readFile(path, 'utf-8') | ||
.then(contents => ({ path, contents })); | ||
}); | ||
return Promise.all(fileReadPromises); | ||
return Promise.all(paths.map(path => { | ||
return readFile(path, encoding) | ||
.then(contents => { | ||
contents = contentFn(contents, path); | ||
return { path, contents }; | ||
}); | ||
})); | ||
}); | ||
} | ||
|
||
/** | ||
* Read the files from a glob, but then instead of resolving the | ||
* Promise with an Array of objects (@see readFiles), resolve with a | ||
* single object; each file's contents is keyed by its filename run | ||
* through keyname(). | ||
* through optional `keyFn(filePath, options)`` (default: keyname). | ||
* Will pass other options on to readFiles and keyFn | ||
* | ||
* @param {glob} | ||
* @param {Object} options (all optional): | ||
* - keyFn | ||
* - contentFn | ||
* - stripNumbers | ||
* @return {Promise} resolving to {Object} of keyed file contents | ||
*/ | ||
function readFilesKeyed (glob, preserveNumbers = false) { | ||
return readFiles(glob).then(allFileData => { | ||
function readFilesKeyed (glob, options = {}) { | ||
const { | ||
keyFn = (path, options) => keyname(path, options) | ||
} = options; | ||
return readFiles(glob, options).then(allFileData => { | ||
const keyedFileData = new Object(); | ||
for (var aFile of allFileData) { | ||
keyedFileData[keyname(aFile.path, preserveNumbers)] = aFile.contents; | ||
keyedFileData[keyFn(aFile.path, options)] = aFile.contents; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will default to using |
||
} | ||
return keyedFileData; | ||
}); | ||
|
@@ -65,15 +103,15 @@ function readFilesKeyed (glob, preserveNumbers = false) { | |
* partials, etc, based on a filepath: | ||
* - replace whitespace characters with `-` | ||
* - use only the basename, no extension | ||
* - unless preserveNumbers, remove numbers from the string as well | ||
* - unless stripNumbers option false, remove numbers from the string as well | ||
* | ||
* @param {String} str filepath | ||
* @param {Boolean} preserveNumbers | ||
* @param {Object} options | ||
* @return {String} | ||
*/ | ||
function keyname (str, preserveNumbers = false) { | ||
function keyname (str, { stripNumbers = true } = {}) { | ||
const name = basename(str).replace(/\s/g, '-'); | ||
return (preserveNumbers) ? name : removeNumbers(name); | ||
return (stripNumbers) ? removeLeadingNumbers(name) : name; | ||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
ding: | ||
- dong | ||
- dell | ||
- 'cat is in the well' | ||
- 5 | ||
forestry: | ||
fob: 'key' | ||
bork: 'bing' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"foo" : "bar", | ||
"fortunately": 5 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
foo: | ||
- bar | ||
- baz | ||
elfin: 'small things' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,21 @@ describe ('utils', () => { | |
expect(badGlobs.every(glob => utils.isGlob(glob))).to.be.false; | ||
}); | ||
}); | ||
describe('keyname', () => { | ||
it ('should strip leading numbers by default', () => { | ||
var result = utils.keyname('foo/01-bar.baz'); | ||
expect(result).not.to.contain('01-'); | ||
}); | ||
it ('should strip parent directories and extensions', () => { | ||
var result = utils.keyname('foo/01-bar.baz'); | ||
expect(result).not.to.contain('foo'); | ||
expect(result).not.to.contain('baz'); | ||
}); | ||
it ('should accept option to retain leading numbers', () => { | ||
var result = utils.keyname('foo/01-bar.baz', { stripNumbers: false }); | ||
expect(result).to.contain('01-'); | ||
}); | ||
}); | ||
describe('readFiles', () => { | ||
it ('should read files from a glob', done => { | ||
var glob = path.join(__dirname, 'fixtures/helpers/*.js'); | ||
|
@@ -65,14 +80,53 @@ describe ('utils', () => { | |
done(); | ||
}); | ||
}); | ||
it ('should be able to key files by getName', done => { | ||
it ('should run passed function over content', done => { | ||
var glob = path.join(__dirname, 'fixtures/helpers/*.js'); | ||
utils.readFiles(glob, { contentFn: (content, path) => 'foo' }) | ||
.then(allFileData => { | ||
expect(allFileData).to.have.length.of(3); | ||
expect(allFileData[0].contents).to.equal('foo'); | ||
done(); | ||
}); | ||
}); | ||
}); | ||
describe('readFilesKeyed', () => { | ||
it ('should be able to key files by keyname', done => { | ||
var glob = path.join(__dirname, 'fixtures/helpers/*.js'); | ||
utils.readFilesKeyed(glob).then(allFileData => { | ||
expect(allFileData).to.be.an('object'); | ||
expect(allFileData).to.contain.keys('toFraction', 'toJSON', 'toSlug'); | ||
done(); | ||
}); | ||
}); | ||
it ('should accept an option to preserve leading numbers', done => { | ||
var glob = path.join(__dirname, 'fixtures/data/*.yaml'); | ||
utils.readFilesKeyed(glob, { stripNumbers: false }).then(allFileData => { | ||
expect(allFileData).to.be.an('object'); | ||
done(); | ||
}); | ||
}); | ||
it ('should accept a function to derive keys', done => { | ||
var glob = path.join(__dirname, 'fixtures/data/*.yaml'); | ||
utils.readFilesKeyed(glob, { keyFn: (path, options) => 'foo' + path }) | ||
.then(allFileData => { | ||
expect(Object.keys(allFileData)[0]).to.contain('foo'); | ||
done(); | ||
}); | ||
}); | ||
it ('should pass contentFn through to readFiles', done => { | ||
var glob = path.join(__dirname, 'fixtures/data/*.yaml'); | ||
utils.readFilesKeyed(glob, { | ||
keyFn: (path, options) => 'foo' + path, | ||
contentFn: (content, path) => 'foo' | ||
}).then(allFileData => { | ||
for (var fileKey in allFileData) { | ||
expect(fileKey).to.contain('foo'); | ||
expect(allFileData[fileKey]).to.equal('foo'); | ||
} | ||
done(); | ||
}); | ||
}); | ||
}); | ||
describe('parent directory (parentDirname)', () => { | ||
it ('should derive correct parent dirname of files', () => { | ||
|
@@ -82,7 +136,7 @@ describe ('utils', () => { | |
}); | ||
}); | ||
describe('merge()', () => { | ||
it ('works', () => { | ||
it ('merges objects correctly', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👺 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👅 |
||
var actual = utils.merge( | ||
{a: 1, c: {d: 3}}, | ||
{a: 2, b: 1, c: {e: 4}} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
can now take aparseFn
to run over data files. This allows a user to parse with something else, e.g.JSON
or something.