wizicer / gulp-csvtojson

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

Remove option to create JS file #3

Closed tjchaplin closed 8 years ago

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 will open up another issue 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

Good suggestion, I agree with you, this is better than the redundant option.

To assist the consumer who also may have such type of requirement, we can write them as standard usage in README.

wizicer commented 8 years ago

I tried remove option and created PR #4, please take a look, and let me know your suggestion or concern.

wizicer commented 8 years ago

4 has been merged, thus close this issue.