WIP Remove shelljs #79
Conversation
Merging in changes
Merging in Dmitriz's changes
Running npm run coverage will now generate a HTML coverage report.
Coverage identified that gentle-copy.read was uncovered by tests
![codelingo[bot]](https://avatars.githubusercontent.com/in/14339?s=60&v=4){kind=small}
|
Looks awesome, thanks, will have a look. |
|
Thanks again! Seeing you are using What do you think of instead exporting plain callback style function? |
|
I've only been doing nodejs for a little over a year (i'm a c# guy really) - and when I started the lead programmer who mentored me was a user of async/await - and this has sort of left me reaching for async and await by default. I've never done callback functions, and if you want I'll have a go a refactoring the code to use callbacks. leave it with me and I'll try a better version tonight - I'm in the UK, and it's a public holiday tomorrow, so I'll have to try and get it done tonight because I have a feeling that there may be plans for the Monday. |
|
Yes, you may find Node.js more of wild west than other more established languages. Some years ago promises were the next hot thing, while few years thereafter people realized the design wasn't that convenient, which is how the async/await came up, to make the async code look the same as sync. While callbacks are simpler and more powerful, especially with arrow functions and currying: Let me know in case of any difficulties... |
* Some FS commands (exist etc) were asynchronous - replaced these with Synchronous versions * Replaced the mkdir with recursive option to ensureDireSync * Tests pass 100%
Sorry - My IDE had coloured the path reference (for example) as grey - indicating that it wasn't actually used anymore - but I hadn't spotted it. In previous projects I've used a eslinter - deepscan.io provides this functionality - thankfully - but maybe we could add eslint support?
|
Hi @dmitriz - I'll fix the hound issues. Sorry I didn't realize that hound was a thing... |
|
Thanks a lot and apologies for the crazy bot, please ignore it! |
* Moved some of the variable declaration around destination. Destination doesn't change for each file, therefore no need to define these every iteration.
|
Looks great, is the |
|
Sorry to hear, I've thought we were almost there...
You essentially have it, just need to change to the callback versions.
Anyway, if you change your mind, I'll be happy to accept such PR.
…On Tue, May 28, 2019 at 8:54 PM Mike Hingley ***@***.***> wrote:
I'm going to close this pull request - I think there is still too much to
do
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AATFSKMFKRIPVAFLSEK2RATPXU2PTA5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWMGIUQ#issuecomment-496526418>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AATFSKIMUCGMNBOOSHXAGELPXU2PTANCNFSM4HPVMMQQ>
.
--
Dmitri Zaitsev
School of Mathematics
Trinity College Dublin
WWW: http://www.maths.tcd.ie/~zaitsev/
|
Main page for this project suggests that it supports standard - it isn't installed so installing it. @dmitriz mentioned in a previous commit : Eslint and the likes are exactly the tasks I am trying to offload to the bots :) That way I don't need to bloat my packages, keeping and maintaining more dependencies can become a pain. Also Eslint specifically had a high profile security incident and being that popular is attractive target for hackers. My thoughts were : Using bots to enforce Standard JS Code standards is fine - but ensuring that code is 'correct' replies upon me pushing the code to github, only to then find that it fails code standards. As the project shows compliance with Standard, and that standard can be used to check the code conformance I think that adding in standard would be a good thing to do - it allows me to check my code before uploading. If this is a bad idea then this commit can be reverted out - but I find it useful to know that my code is compliant before pushing.
Hi @dmitriz - In the index.js file the read function exposed there uses |
|
Ah yes, you are right, it is been a while ago, it did somehow work, but now
that you have discovered that hole
and documented in the comments, it occurred to me that maybe the sync
version shouldn't be used in the tests in first place. Because the tests
are unreliable otherwise.
And yes, a cleaner way would be to use the async versions instead I think,
it is very similar except that you have the callback where you know for
sure the process was completed so you can reliably test.
…On Tue, May 28, 2019 at 9:47 PM Mike Hingley ***@***.***> wrote:
In the read file function, the problem with inaccessible file seems that
the sync version fs.readFileSync is used. It feels a bit unnatural in
Node.js, where we have fs.readFile with callbacks providing the needed
control flow, whereas with the sync version you have no way to know when
the process is finished, also we want to keep our tests swift and
non-blocking.
Hi @dmitriz <https://github.com/dmitriz> - In the index.js file the read
function exposed there uses fs.readFileSync - This was the reason why I
wrote this read method here - similar to the method that the main library
exposes. Do you think we should replace that with the async version (with
call back support) also?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AATFSKMPQZJCTZTOMS7HYOTPXVAYRA5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWMLZVY#issuecomment-496549079>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AATFSKOAZEAN635TROKZ4YLPXVAYRANCNFSM4HPVMMQQ>
.
--
Dmitri Zaitsev
School of Mathematics
Trinity College Dublin
WWW: http://www.maths.tcd.ie/~zaitsev/
|
|
Hi @dmitriz - I've had a go at callbacks - and I've really struggled - particularly when comparing the output of one file, with another - using callbacks. so the tests (currently) look like this : whereas I think in a callback solution we'd need to do something like this : and if you want to compare one file with another then this could be problematic.
|
|
It is basically the problem caused by the sync methods. E.g. you cannot write await fs.writeFileSync(...)The sync methods are meant to imitate the shell methods but they do not wait for the execution, so are not good fit generally for anything advanced. This is also one of the reasons Callbacks or promises are better in that respect, test('copy one directory preserving file structure', t => {
mkdir('tmp/dir_old')
write('tmp/dir_old/file', 'mytext')
...Here the test('copy one directory preserving file structure', t => {
fs.mkdir('tmp/dir_old')
.then(() => fs.write('tmp/dir_old/file', 'mytext'))
.then(... another function returning promise ... )
...or with callbacks simply test('copy one directory preserving file structure', t => {
fs.mkdir('tmp/dir_old', () => {
fs.write('tmp/dir_old/file', 'mytext'), () => {
... anything can go here ...
}
}
}
...So comparing with promises, you don't even need to wrap into promise returning functions. Of course, for anything advanced this nesting can get in your way, which is why I am working on this: https://github.com/dmitriz/cpsfy |
… using colorfully or coloUrfully
|
Hi, I've read your comments, it is all fine, It could be easier to commit stuff unrelated to removing shelljs in separate PRs, Basically don't worry about anything, including the bots, their role is advisory - feel free to disregard. The main job is to get rid of the shelljs and keep the main functionality. |
|
thanks @dmitriz - I'm determined to get callbacks working for you - and I've just managed to get a test working using callbacks - I'm going to fix all the tests to use the same mechanism, and I'll replace any synchronous file system calls I can find - like you identified the appendFile. Sorry this PR has been so difficult. If you want I'll separate out the essential async + shellJS stuff from anything else I'm doing here (like lint stuff etc) |
|
No problem and thanks for your help.
We can quickly import any unrelated stuff not affecting basic functionality
in separate PRs if that helps you save time.
…On Wed, May 29, 2019 at 4:52 PM Mike Hingley ***@***.***> wrote:
thanks @dmitriz <https://github.com/dmitriz> - I'm determined to get
callbacks working for you - and I've just managed to get a test working
using callbacks - I'm going to fix all the tests to use the same mechanism,
and I'll replace any synchronous file system calls I can find - like you
identified the appendFile.
Sorry this PR has been so difficult. If you want I'll separate out the
essential async + shellJS stuff from anything else I'm doing here (like
lint stuff etc)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AATFSKPBQGML6VBZVUOR5TTPXZG5ZA5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWOZW2Y#issuecomment-496868203>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AATFSKPD6ECUDH5KOCUBVMDPXZG5ZANCNFSM4HPVMMQQ>
.
--
Dmitri Zaitsev
School of Mathematics
Trinity College Dublin
WWW: http://www.maths.tcd.ie/~zaitsev/
|
|
This might be a helpful guide: |
|
Hi Dimitri...
I'll take a look when I can but my wife and I are just expecting the birth of our baby...
I'm in the delivery suite with her at the moment.
Sent from my Samsung Galaxy S6 - powered by Three
…-------- Original message --------
From: Dmitri Zaitsev <notifications@github.com>
Date: 04/06/2019 03:46 (GMT+00:00)
To: dmitriz/gently-copy <gently-copy@noreply.github.com>
Cc: Mike Hingley <computa_mike@hotmail.com>, State change <state_change@noreply.github.com>
Subject: Re: [dmitriz/gently-copy] Remove shelljs (#79)
This might be a helpful guide:
https://areknawo.com/node-js-file-system-api-beginner-friendly-guide/
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<#79?email_source=notifications&email_token=AADRP3AISOZ2SUMOVHOO3JLPYXJQZA5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3HVRQ#issuecomment-498498246>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AADRP3GI6B4G3XLIGEALECDPYXJQZANCNFSM4HPVMMQQ>.
|
|
No worries, babies come first, congratulations! :)
…On Thu, Jun 6, 2019 at 2:05 PM Mike Hingley ***@***.***> wrote:
Hi Dimitri...
I'll take a look when I can but my wife and I are just expecting the birth
of our baby...
I'm in the delivery suite with her at the moment.
Sent from my Samsung Galaxy S6 - powered by Three
-------- Original message --------
From: Dmitri Zaitsev ***@***.***>
Date: 04/06/2019 03:46 (GMT+00:00)
To: dmitriz/gently-copy ***@***.***>
Cc: Mike Hingley ***@***.***>, State change <
***@***.***>
Subject: Re: [dmitriz/gently-copy] Remove shelljs (#79)
This might be a helpful guide:
https://areknawo.com/node-js-file-system-api-beginner-friendly-guide/
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub<
#79?email_source=notifications&email_token=AADRP3AISOZ2SUMOVHOO3JLPYXJQZA5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODW3HVRQ#issuecomment-498498246>,
or mute the thread<
https://github.com/notifications/unsubscribe-auth/AADRP3GI6B4G3XLIGEALECDPYXJQZANCNFSM4HPVMMQQ
>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79?email_source=notifications&email_token=AATFSKKYT2XYKLWX66DWDWDPZCZK3A5CNFSM4HPVMMQ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXB53DA#issuecomment-499375500>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AATFSKJNRXNPHXAMDEB5MWLPZCZK3ANCNFSM4HPVMMQQ>
.
--
Dmitri Zaitsev
School of Mathematics
Trinity College Dublin
WWW: http://www.maths.tcd.ie/~zaitsev/
|
|
Any updates on this? Just tried to upgrade node from v13 to v14/15, and getting a whole bunch of warnings, which seem to come from Trace warnings from running a script using `gently-copy`
Usages of shelljs
|
|
PRs are welcome 🙂 |
Adding more tests, removing shelljs.
I also made the tests run serially. The reason I thought this would be a good idea because if a test runs in parallel then it might be that one tests execute as another is executing - and the file system is a shared dependency.
A future improvement might be an in-memory file system - potentially allow the tests to execute in parallel.
I've added a coverage report option - run
npm run coveragewill generate a HTML report in the coverage folder. When I ran this report I noticed that the read function was not covered by a test. I added a sample UTF-8 file and a test. Coverage is now at 100%.