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

Data and File Parsing #6

Merged
merged 16 commits into from
Mar 4, 2016
Merged

Data and File Parsing #6

merged 16 commits into from
Mar 4, 2016

Conversation

lyzadanger
Copy link
Contributor

This PR introduces a bunch of generalized utility that will make it easier to parse various kinds of files and specifically handles the parsing of data files. It handles some options changes and a lot of tests.

/cc @erikjung @mrgerardorodriguez

import { merge } from './utils';

const defaults = {
data: {
src: ['src/data/**/*.yaml'],
parseFn: (contents, path) => yaml.safeLoad(contents)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

options can now take a parseFn to run over data files. This allows a user to parse with something else, e.g. JSON or something.

@lyzadanger
Copy link
Contributor Author

Hmmm, commit history looks a bit funky. The diffs look correct, however.

return dirname(filepath).split(path.sep).pop();
}
function removeLeadingNumbers (str) {
return str.replace(/^[0-9|\.\-]+/, '');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add @examples to these functions to demonstrate the I/O?

Also, for removeLeadingNumbers, can we broaden the delimiters it will catch? I'm not sure what the original Fabricator did, but I would expect all of these to match:

screen shot 2016-03-04 at 2 36 30 pm

Here's another pattern that might work:

screen shot 2016-03-04 at 2 44 06 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed some comments/examples on most of those utility functions.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

@erikjung
Copy link
Contributor

erikjung commented Mar 4, 2016

@lyzadanger Looks good. I think it'd be nice to auto-generate some docs for this.

@lyzadanger
Copy link
Contributor Author

@erikjung Yes, I'd like to generate some docs. You wanna take that on at some point?

@erikjung
Copy link
Contributor

erikjung commented Mar 4, 2016

@lyzadanger Yeah, I'll make an issue.

@gerardo-rodriguez
Copy link
Member

Looks good overall. Had to take a bit longer to get through it, but I generally process things slower.
¯_(ツ)_/¯

@lyzadanger
Copy link
Contributor Author

OK, I'm gonna merge for now!

lyzadanger added a commit that referenced this pull request Mar 4, 2016
@lyzadanger lyzadanger merged commit 5c72725 into feature-rewrite Mar 4, 2016
@lyzadanger lyzadanger deleted the feature-data branch March 4, 2016 23:06
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.

3 participants