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

readyFor limited time #3

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

Conversation

Freak613
Copy link

Added readyFor function.
It may be useful when between last and next requests some handling happens and we need to wait some time to make sure that all done.

@alfonso-presa
Copy link
Owner

Thank you very much for your contribution. The code looks ok, but I was wondering which would be a concrete functional use case for this feature... ¿Could you provide one please?

I ask this because as for your implementation it looks like readyFor will wait for the given amount of time and if nothing is pending, it will execute the callback. But according to the definition you made it should ensure that "no new action has started in specified time period". As far as I see in the implementation, that's not completely true as if something has been started, but has also finish in the given interval, the callback will execute ignoring the fact that something was started.

So:

  • Should the description be corrected to reflect that readyFor is just a delayed equivalent for ready? In this case I'm not sure it the readyFor method is needed, as this same thing could be achieved with a timeout and a ready call (maybe a isReady method providing the current application status synchronously would be nice to cover this scenario).
  • or should the implementation be changed so that if something happens during the given interval, it waits again until the application keeps in a "ready" state for the given amount of time? In this case some kind of event should be triggered when the application gets "unready" so that the timeout get's cancelled and the readyFor is invoked again. There should be some tests added to ensure that this works correctly.
  • or... may be I didn't understood the use case at all ^_^...

Thanks!

@Freak613
Copy link
Author

The goal is "no new action has started in specified time period".
I made fixes for mentioned case and cover new func with tests.

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.

2 participants