wizicer / gulp-csvtojson

csv to json converter for gulp
MIT License
3 stars 1 forks source link

Updated to include streams #2

Closed tjchaplin closed 8 years ago

tjchaplin commented 8 years ago

Issue #1 - Support streams.

tjchaplin commented 8 years ago

Updated per comment.

Also, refactored the tests and index to allow for more readability.

tjchaplin commented 8 years ago

Further reviewing the code...it seems like there are too many paths for the plugin. Ideally there should be a single purpose; i.e. stream csvtojson module using gulp. The current plugin is streaming, but also will create a js file depending on options. The creation of a JS file could(should) be done in a different plugin. Here is the gulp plugin guidelines which provides a little more detail: Gulp Plugin Guidelines

I have opened up another issue(#3) to discuss further.

Here is what currently exists:

var gulp = require('gulp');
var csvtojson = require('gulp-csvtojson');

gulp.task('default', function () {
    return gulp.src('src/file.csv')
        .pipe(csvtojson())
        .pipe(gulp.dest('dist'));
});

I think this should be:

var gulp = require('gulp');
var csvtojson = require('gulp-csvtojson');
var insert = require('gulp-insert'); //module to insert into streamed data

gulp.task('default', function () {
    return gulp.src('src/file.csv')
        .pipe(csvtojson())
        .pipe(insert.prepend('var anyVariable = '))
        .pipe(insert.append(';'))
        .pipe(gulp.dest('dist'));
});

Gulp Insert plugin

wizicer commented 8 years ago

+1 for separating fixtures from cases, however, obvious code duplication between buffers and streams directory which also produces problematic code twice.

Anyway, I suggest for this PR, let's just focus on this stream support, let's merge the PR after the two issues I just raise has been resolved. Then we can continue enhance with less code duplication later.

tjchaplin commented 8 years ago

Yes agree on some duplication...and we can review that. I think if the streams are pushed in. Maybe work on #3 , and remove tests/duplication from there.

Let me know if anything else.