find-remove

find-remove copied to clipboard

Hi, tried my take at adding async support. This requires node 8 with async/await because this allows the code to be really close. The only test I needed to change (apart from the .then) was adding setTimeout with 1 ms for the 0.0005 tests.

What do you think?

thanks for your efforts - a couple of problems with your commits:

• do not rename file otherwise git history is lost
• in general i disklike promises. too magical - agree current code is callback hell. but prefer streams/events instead here
• why had the tests to be changed re: setTimeout () - isn't that like changing the contract?
• to have travis pick the correct node js version, mention it in the yml file, see example https://github.com/binarykitchen/videomail-client/blob/develop/.travis.yml#L3

I see. Well I can't think of any maintainable way to do these complicated checks with callbacks (as you mentioned) - at the same time I can't envision how it should work with streams / and or events. Perhaps you can share some ideas? I work mostly nowadays with async/await and really like it, it's just so much more readable and logic then all these callbacks (or even promises).

The other points:

1. I thought you might want to keep both versions - but changed, so now the history is visible
2. I "unchanged" the 0.0005 seconds test and they still failed on my system - but on travis it's not a problem. I rechecked and even before my change these tests fail on my system (mac os x).This seems to be about the speed of the system? I added some logging:

path | now - expiration time | now | mtime | now > expirationTime
path .../directory1/directory1_2/directory1_2_1/something.jpg ageSeconds 0.5 1518528404355 1518528404354  _  true
path .../find-remove/directory1/directory1_2/directory1_2_1/something.png ageSeconds -0.5 1518528404355 1518528404355  _  false

Seems to be that the files are not old enough in these cases and thus not deleted. It would help to change the check from return now > expirationTime to >=.

4. Thanks for the info, now the build runs fine.

great, looking much better. we're close.

about the 0.0005 ... can you change it to a constant at top of test and try to increment it slowly until all test pass. very curious here. dont change to >= as this isn't correct.

and yes, don't forget documentation ;)

I think the problem with the 0.0005s check is that it's already higher than the available precision. Or am I overlooking something? From my example:

current time:       1518528404355
modification time:  1518528404355
expiration time is  1518528404355.5
-> difference is    -0.5

current and modification time is already exactly the same number. Might work to switch to process.hrtime. But currently out of time for the week, so more next week.

or it is because we have different CPU speeds? what's your CPU?

It's a 2,6 GHz Intel Core i5 / MBP from 2014 with 10.13.3

okay your machine is faster than mine, although tests pass here. are you on mac os?

Yes, I am on mac os x. I think it's something with the speed as well. My machine "is too" fast in a way, because the files are not old enough to be older than 0.0005s. Thus a setTimeout of 1ms in the test would ensure that the files are old enough to be watched by the module.

ok, understand. in that case change increase slightly, maybe to 1ms or more until all tests pass.

@davidlanger you still alive?

Yepp, but was on vacation the last two weeks. Will continue as fast as possible.

But you wrote to "increase slightly, maybe to 1ms or more". But this won't help the problem, as the problem is that the files are not old enough. The only way to make this check work, is to add timeout, as the problem is that on my computer the files are never old enough to be deleted if the code is run right after the initialization.

Illustration: Currently:

1. Initialization, creating files
2. Test

The time between 1 and 2 is less than the precision available (which is only in milliseconds). 0.0005s is less than a millisecond.

With set Timeout:

1. Initialization
2. Wait 1ms
3. Test

Now all the newly created files are old enough.

oh i see, sorry - in that case add the timeout then

@davidlanger hello?

@davidlanger you still alive?

Hello, Any news of this?

Cheers,

@Hibrix-net sorry no time and needs rebasing

@mgerylrm thanks for your approval, yet merge conflicts should be resolved first.