path get/set for recording path change #24
Conversation
| }, | ||
| set: function(path) { | ||
| if (!this.history) { | ||
| this.history = []; |
There was a problem hiding this comment.
do this in the constructor, no reason to do it lazily
|
OK, I have update this PR. |
| this.history.push(path); | ||
| } | ||
|
|
||
| this._path = path; |
There was a problem hiding this comment.
not needed? _path shouldnt exist anymore
| @@ -11,6 +11,9 @@ var cloneBuffer = require('./lib/cloneBuffer'); | |||
| function File(file) { | |||
| if (!file) file = {}; | |||
|
|
|||
| // record path change | |||
| this.history = []; | |||
There was a problem hiding this comment.
this.history = [file.path] then ditch the this.path == file.path || null
There was a problem hiding this comment.
file.path will be null, then this.history = file.path ? [file.path] : [], is that ok?
| return this.history[this.history.length - 1]; | ||
| }, | ||
| set: function(path) { | ||
| // record history only when path changed |
There was a problem hiding this comment.
check for typeof string on path and throw if its not? this would make the most sense IMO vs. doing null checks
There was a problem hiding this comment.
Should I check empty string?
The condition is typeof path === 'string' && path, any suggestion?
There was a problem hiding this comment.
Empty string will be ignore when set the path, needn't check. Now updated
|
@contra code has been updated follow your suggestion |
|
ping @contra |
|
Still thinking about it |
| @@ -11,12 +11,13 @@ var cloneBuffer = require('./lib/cloneBuffer'); | |||
| function File(file) { | |||
| if (!file) file = {}; | |||
|
|
|||
| // record path change | |||
| this.history = file.path ? [file.path] : []; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@doowb file.path will never be an array though, file.history will though
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@doowb clone will have to be rewritten for custom properties and history support
|
Any response? |
|
How about this PR? |
path get/set for recording path change
|
Still need to update clone to support copying history |
|
👍 I will send another PR about this. |
|
@popomore There are 3 open pull requests and a bunch of issues for clone. If you can roll them all up into one pull request with tests I'll buy you a drink 🍻 |
|
All right 🍻 |
related issues: * <jgable/gulp-cache#30> * <gulpjs/vinyl#24>
path get/set for recording path change
Implement for #19 (comment)