-
-
Notifications
You must be signed in to change notification settings - Fork 107
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
Add dirname, basename and extname getters #46
Conversation
@@ -158,6 +158,36 @@ Object.defineProperty(File.prototype, 'relative', { | |||
} | |||
}); | |||
|
|||
Object.defineProperty(File.prototype, 'dirname', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file.base already exists, using dirname for output would be an antipattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dirname has a different meaning and can actually be different than file.base. For example:
gulp.src('assets/css/**/*.scss')
Every file's file.base will be assets/css
but any file in a subdirectory will have a different dirname.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Yup, different use-case.
👍 This would be very handy. I would especially like being able to do `file.extension = '.foo' to change the extension, instead of having to use the gutil helper. So everything should have setters too IMHO. |
@phated thoughts? |
return path.extname(this.path); | ||
}, | ||
set: function() { | ||
throw new Error('File.extname is generated from the base and path attributes. Do not modify it.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i do agree that if we have a setter for this it should use the replace-ext module on path
Right on, I've added setters for all of these properties. What do you guys think? |
return path.extname(this.path); | ||
}, | ||
set: function(extname) { | ||
this.path = replaceExt(this.path, extname); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setters should also throw the errors on no path, or just do nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and personally prefer that they throw. Added.
f869ff6
to
5a41647
Compare
@jeremyruppel Can you also update the docs? |
b5f71cb
to
d8f8497
Compare
Just squashed these commits. @contra @sindresorhus @phated thoughts? |
@jeremyruppel Rebase? |
d8f8497
to
e09817f
Compare
@contra yep! README conflicts. Sorry about the trailing whitespace stuff. |
Add dirname, basename and extname getters
Thanks @jeremyruppel ! |
Thanks @contra & @sindresorhus! |
@@ -38,42 +38,42 @@ var coffeeFile = new File({ | |||
|
|||
#### options.cwd | |||
|
|||
Type: `String` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this whitespace should change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Markdown treats double trailing whitespace as a newline. It's stupid, but @stevemao is right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point. #52
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
Add dirname, basename and extname getters
Not sure if you guys would consider this bloat or a useful feature, but I often find myself wanting these pathname-style helpers when working with vinyl Files. Cheers!