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.

Sign up for GitHub

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

Jump to bottom

path get/set for recording path change #24

Merged
merged 4 commits into from
Aug 28, 2014
Merged

path get/set for recording path change #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f3f9be0
path get/set for recording path change #19
popomore Jun 28, 2014
d95023f
delete this._path :tongue:
popomore Jun 28, 2014
7c71bf3
remove initialize of this.path
popomore Jun 28, 2014
e50ceac
throw when set path is null
popomore Jun 29, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ var cloneBuffer = require('./lib/cloneBuffer');
function File(file) {
if (!file) file = {};

// record path change
this.history = file.path ? [file.path] : [];
Copy link
Member

Choose a reason for hiding this comment

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

Only noticed this because of the clone issue... I think this line should be
this.history = file.path ? (Array.isArray(file.path) ? file.path : [file.path]) : [];
This allows using clone like it is below.

Copy link
Member

Choose a reason for hiding this comment

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

@doowb file.path will never be an array though, file.history will though

Copy link
Member

Choose a reason for hiding this comment

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

That's right. I was wondering how history would be cloned when I was looking at this and I think it messed up how I was thinking of this.path.

Copy link
Member

Choose a reason for hiding this comment

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

@doowb clone will have to be rewritten for custom properties and history support


// TODO: should this be moved to vinyl-fs?
this.cwd = file.cwd || process.cwd();
this.base = file.base || this.cwd;

this.path = file.path || null;

// stat = fs stats object
// TODO: should this be moved to vinyl-fs?
this.stat = file.stat || null;
Expand Down Expand Up @@ -123,4 +124,18 @@ Object.defineProperty(File.prototype, 'relative', {
}
});

Object.defineProperty(File.prototype, 'path', {
get: function() {
return this.history[this.history.length - 1];
},
set: function(path) {
if (typeof path !== 'string') throw new Error('path should be string');

// record history only when path changed
Copy link
Member

Choose a reason for hiding this comment

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

check for typeof string on path and throw if its not? this would make the most sense IMO vs. doing null checks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I check empty string?

The condition is typeof path === 'string' && path, any suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empty string will be ignore when set the path, needn't check. Now updated

if (path && path !== this.path) {
this.history.push(path);
}
}
});

module.exports = File;
65 changes: 62 additions & 3 deletions test/File.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ describe('File', function() {
done();
});
});

describe('isBuffer()', function() {
it('should return true when the contents are a Buffer', function(done) {
var val = new Buffer("test");
Expand Down Expand Up @@ -382,7 +382,7 @@ describe('File', function() {
process.nextTick(done);
});
});

describe('inspect()', function() {
it('should return correct format when no contents and no path', function(done) {
var file = new File();
Expand Down Expand Up @@ -445,7 +445,7 @@ describe('File', function() {
done();
});
});

describe('contents get/set', function() {
it('should work with Buffer', function(done) {
var val = new Buffer("test");
Expand Down Expand Up @@ -537,4 +537,63 @@ describe('File', function() {
});
});

describe('path get/set', function() {

it('should record history when instantiation', function() {
var file = new File({
cwd: '/',
path: '/test/test.coffee'
});

file.path.should.eql('/test/test.coffee');
file.history.should.eql(['/test/test.coffee']);
});

it('should record history when path change', function() {
var file = new File({
cwd: '/',
path: '/test/test.coffee'
});

file.path = '/test/test.js';
file.path.should.eql('/test/test.js');
file.history.should.eql(['/test/test.coffee', '/test/test.js']);

file.path = '/test/test.coffee';
file.path.should.eql('/test/test.coffee');
file.history.should.eql(['/test/test.coffee', '/test/test.js', '/test/test.coffee']);
});

it('should not record history when set the same path', function() {
var file = new File({
cwd: '/',
path: '/test/test.coffee'
});

file.path = '/test/test.coffee';
file.path = '/test/test.coffee';
file.path.should.eql('/test/test.coffee');
file.history.should.eql(['/test/test.coffee']);

// ignore when set empty string
file.path = '';
file.path.should.eql('/test/test.coffee');
file.history.should.eql(['/test/test.coffee']);
});

it('should throw when set path null', function() {
var file = new File({
cwd: '/',
path: null
});

should.not.exist(file.path)
file.history.should.eql([]);

(function() {
file.path = null;
}).should.throw('path should be string');
});
});

});