Added the ability to preserve file mode on method actions.template #474
Conversation
|
Changes Unknown when pulling 63d8abf on ruyadorno:iss-464 into * on yeoman:master*. |
| this.checkForCollision(filepath, content, function (err, config) { | ||
| if (err) { | ||
| config.callback(err); | ||
| return this.emit('error', err); | ||
| } | ||
|
|
||
| var fileOptions = {}; |
There was a problem hiding this comment.
The code will be cleaner if you accept a writeFile option hash as third parameter rather than only the file mode.
|
Hey Ruy, thanks for taking time to handle this issue! On the concept though, I don't believe we should replicate the same permissions from the file inside the source directory. This will break for people relying on correctly setup machine with umask and set group id permissions. Rather, I believe we should only replicate the executable bits. ( |
|
I though about it a little more, and maybe permissions issues aren't going to be a problem after all. I've been working on File-utils (instantiated as the new Worst case scenario, we'll update if people got some issues with his. So, just fix the method to take an option hash and we'll be good to merge. |
|
Great! Thanks for taking the time to review this too! I honestly haven't thought through all possible permission issues but great to see that it will eventually make it to the code :) I'll send the commit fixing the option. |
| @@ -201,6 +201,7 @@ describe('yeoman.generators.Base', function () { | |||
| describe('without options', function () { | |||
| before(function (done) { | |||
| this.dummy.foo = 'fooooooo'; | |||
| this.dummy.template('foo.js', 'write/to/from-foo.js'); | |||
There was a problem hiding this comment.
Could you instead writeFileSync this file with a file mode that you wouldn't usually found ? (e.g. parseInt(337, 8)).
|
LGTM, can you squash your commits together and rebase on latest master? ( |
|
Ok, I changed the test file to look like what you've said, only thing that is bothering me is that this test is kind of looking different from all the other ones who uses files already present on Also I had to dispose of the file after... tell me what you think, if everything is good I'll look into squashing the commits |
| @@ -201,9 +201,10 @@ actions.read = function read(filepath, encoding) { | |||
| * | |||
| * @param {String} filepath | |||
| * @param {String} content | |||
| * @param {String} writeFile | |||
There was a problem hiding this comment.
This will be an object ;) Maybe add a link to Node.js writeFile documentation.
…ates to yeoman#464 Preserve file mode on actions.template, fixes yeoman#464
|
I just pushed the squashed commits rebased after the latest master, let me know if it is good |
|
Perfect, thanks a lot! |
Added the ability to preserve file mode on method actions.template
Added the ability to preserve file mode on method
actions.templateFixes #464