Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise/Async support #13

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open

Conversation

d0b1010r
Copy link

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?

Michael Heuberger and others added 29 commits July 17, 2015 16:11
… always use de mtime. ctime is used when the permissions file changes, but mtime, when the content file is changed.
…le operation. The returned Json will only include files deleted. Tests added too.
…tation

Adding an option to limit the number of files to be deleted at a single operation
…tation

Adding an option to delete by prefix. Tests added too.
@d0b1010r d0b1010r changed the title Add promise Promise/Async support Feb 12, 2018
@binarykitchen
Copy link
Owner

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

@d0b1010r
Copy link
Author

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 >=.

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

@binarykitchen
Copy link
Owner

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.

@binarykitchen
Copy link
Owner

and yes, don't forget documentation ;)

@d0b1010r
Copy link
Author

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.

@binarykitchen
Copy link
Owner

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

@d0b1010r
Copy link
Author

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

@binarykitchen
Copy link
Owner

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

@d0b1010r
Copy link
Author

d0b1010r commented Feb 19, 2018 via email

@binarykitchen
Copy link
Owner

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

@binarykitchen
Copy link
Owner

@davidlanger you still alive?

@d0b1010r
Copy link
Author

d0b1010r commented Mar 7, 2018

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.

@binarykitchen
Copy link
Owner

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

@binarykitchen
Copy link
Owner

@davidlanger hello?

@binarykitchen
Copy link
Owner

@davidlanger you still alive?

@Hibrix-net
Copy link

Hello,
Any news of this?

Cheers,

@binarykitchen
Copy link
Owner

@Hibrix-net sorry no time and needs rebasing

@binarykitchen
Copy link
Owner

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

@binarykitchen binarykitchen force-pushed the master branch 4 times, most recently from 5beec34 to 2059906 Compare September 14, 2024 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants