Robert-W / grunt-ftp-push

Deploy files to an FTP server as part of your Grunt workflow.
MIT License
39 stars 14 forks source link

Implement recursive directory removal #31

Closed almilo closed 8 years ago

almilo commented 9 years ago

Hi guys,

Thanks for this awesome plugin!!

Because I needed to clean up the upload directory before uploading files and after trying other plugins and approaches, I decided to implement a new decorator for jsftp that adds a kind of 'rm -r' command. See it here: jsftp-rmr. I forked this plugin to add the recursive deletion functionality to it and in order to use it right away, I created a tarball (you can ignore it). The diff view shows plenty of changes in code (probably because of the re-formating of my IDE) but the changes are indeed minimal (see lines 11 and from 313 to 334).

It would be great if you would add this functionality to the plugin.

cheers and thanks again, Alberto

Robert-W commented 9 years ago

Hey @almilo, just wanted to update you with the roadmap. 0.4.0 was just released and I didn't have a chance to get this on that release but want to put it on my next one. Im a little tight on time but am looking for a mock ftp server (thinking about this one, https://github.com/sergi/ftp-test-server) I can use in my unit tests for this new feature. If you would like to try setting something up you may be able to get it done sooner than I, if not, I will probably get around to it next weekend and then I can add this to the latest.

almilo commented 9 years ago

Hi @Robert-W, sorry for the late reply but I am at the moment overwhelmed with work. Please, let me know once you have set up the testing infrastructure and I will update the PR and implement the corresponding tests. Something that I hated while implementing: https://github.com/almilo/jsftp-rmr/blob/master/index.js is that the API of jsftp does not use promises. I thought of using: https://github.com/petkaantonov/bluebird/blob/master/API.md#promisification to 'promisify' it first and also thought that it could be interesting for you too (unless you are OK with callbacks ;)

Robert-W commented 9 years ago

Hey @almilo, No worries. Im in a similar boat, I will add the mock ftp-server setup to get things ready for testing as soon as I get some time.

I do typically prefer promises over callbacks but don't mind callbacks. I have been toying with that idea myself for the recursive directory uploads, I think that's my only function using a callback anymore. But was not sure it would be worth adding another dependency just for that.

I typically prefer to use 3rd party libs as is unless there is gonna be a big benefit from wrapping them, or promisifying them in this case. While I do like promises more then callbacks I'm not sure it would impact this module that much since the source code so small, other then having some nicer looking syntax like below:

server.auth(creds.username, creds.password, function(err) { ... });

versus

server.auth(creds.username, creds.password).then(function(err) { ... });

Although I would not rule it out, especially when node 4.0/5.0 (or whatever they label their next stable build with LTS) comes out which will include native es6 promises.

Robert-W commented 9 years ago

Added localTest.js under the test folder which uses the ftp-test-server. Im not sure how I feel about this as when I ran a test with it, it looks like it created files in my local project folder, so if configured incorrectly and your testing the recursive delete, it may go crazy on your file system and delete everything. Also, the test is disabled right now, using xit in mocha will prevent it from running, because it was having issues with Travis CI. I believe it can work with CI once I get the configuration working correctly for Python and Node.js since this local ftp-test-server uses python.

Robert-W commented 8 years ago

Closing due to inactivity.