espruino / EspruinoTools

JavaScript library of tools for Espruino - used for the Web IDE, CLI, etc.
Apache License 2.0
150 stars 89 forks source link

Implementation of a "--download fileName" option #131

Closed jurelik closed 4 years ago

jurelik commented 4 years ago

This pull request introduces a --download fileName option to be used with the EspruinoTools module.

The downloadFile() function is placed inside both terminal() and connect() functions and executed after a successful connection/file upload if the user included the --download option. This currently covers three scenarios:

If the "fileToDownload" is not found a "File not found." message is logged and the code after executed as per the scenarios above.

There should be no conflicts with any other options that the user chooses. The file is currently downloaded to the current directory but we could potentially be fancy and introduce another argument which could specify the output destination.

My only concern is the fact that EspruinoWebIDE relies on the EspruinoTools module heavily (as per README.md) and I'm not sure if this would introduce any breaking changes. Worse case scenario we can check if the user is connecting through a browser and disable the --download fileName option in that case.

gfwilliams commented 4 years ago

Thanks! While the web IDE uses EspruinoTools it doesn't use bin/espruino-cli.js at all so I don't see your changes will be a problem at all.

However I think there may be some gotchas - there are two different types of file, and your code would only support the appendable StorageFile, not the 'normal' one that JS code/etc is normally written to. I think it may also suffer from https://github.com/espruino/EspruinoWebIDE/issues/234

There's already some code in the EspruinoWebIDE for handling downloading files (https://github.com/espruino/EspruinoWebIDE/blob/gh-pages/js/plugins/storage.js#L45-L72) so this sounds like a great excuse to remove some duplication and also fix https://github.com/espruino/EspruinoWebIDE/issues/234

I'll merge this for now, and will then look into replacing your call to Espruino.Core.Utils.executeStatement with something like a Espruino.Core.Utils.downloadFile that the IDE can use too.

jurelik commented 3 years ago

Hi Gordon, apologies for the late reply, had a busy week.

Thanks for looking over the code and merging it. I have noticed you've already gone ahead and created a Espruino.Core.Utils.downloadFile function and fixed the issue with my code only working for StorageFiles - thanks for that!

Have a nice weekend!