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

Add the ability to check if any single queue isn't empty. #5

Merged
merged 3 commits into from
Nov 7, 2013

Conversation

lyudmil
Copy link
Contributor

@lyudmil lyudmil commented Oct 9, 2013

Hello,

AjaxQ has been great for my projects. Thank you very much for writing it.

One of the projects I'm working on utilizes a number of different queues to extract some parallelism out of the program execution while still sending requests in order where necessary. Unfortunately this meant that I ran up against the problem of not having the ability to check if a given queue is still running. I added this in order to serve my project, but it also might be useful for others. Perhaps you'd consider merging it? Of course, I'm open to any suggestions regarding the API...

@aaronm67
Copy link
Contributor

aaronm67 commented Oct 9, 2013

Does it make more sense to make "queue name" an optional parameter to isRunning?

$.ajaxq.isRunning = function(name) {
    if (name) {
        return queues.hasOwnProperty(name);
    }

    for (var i in queues) {
        if (i && $.ajaxq.isRunning(i)) {
            return true;
        }
    }

    return false;
};

@lyudmil
Copy link
Contributor Author

lyudmil commented Oct 9, 2013

That was my initial thought. I didn't do it because it makes the isRunning function a little messy, so I would've refactored it further and it would've been a bigger change. If you generally don't mind incorporating the functionality, I can use the API you suggested, refactor, and send you another pull request.

@bgrins
Copy link
Contributor

bgrins commented Oct 10, 2013

Yes, I think it would be best to add the queue name as an optional parameter to isRunning. If you could update your PR or send a different one, that would be great. I would probably do something a little simpler, maybe like this (even though it duplicates the small check I think it is easier to read):

$.ajaxq.isRunning = function(name) {
    if (name) {
        if (queues.hasOwnProperty(name)) {
            return true;
        }
    }
    else {
        for (var i in queues) {
            if (queues.hasOwnProperty(i)) {
                return true;
            }
        }
    }
    return false;
};

@lyudmil
Copy link
Contributor Author

lyudmil commented Oct 15, 2013

Hello again,

I've updated my branch to include the functionality to check if a particular queue is running without exposing a new function in the API. The isRunning function now takes the queue name as an optional parameter. I hope this is close to what you had in mind - it works great for my project.

@lyudmil
Copy link
Contributor Author

lyudmil commented Oct 29, 2013

Are you reluctant to merge this or just haven't gotten around to reviewing the changes? I'm sorry to bug, I just wanted to see if I should close this request or not.

@bigamil
Copy link

bigamil commented Oct 29, 2013

@lyudmil Hey man I'll get in contact with @thedustinsmith tomorrow and we will look into merging this in, I'm not the main contact on the project we have using this but I'll follow up. Thank you for your contributions!

@thedustinsmith
Copy link
Contributor

Sorry guys, I meant to get to this sooner, but I got busy and forgot about it. I'd be glad to merge it in. @lyudmil, there's an error in your code. Would you mind changing checkIfQueueRunning to isQueueRunning , then I can get this merged in.

@thedustinsmith thedustinsmith merged commit f397186 into Foliotek:master Nov 7, 2013
@thedustinsmith
Copy link
Contributor

Nevermind @lyudmil - thanks for the help. Sorry it took so long to get merged in.

@lyudmil
Copy link
Contributor Author

lyudmil commented Nov 9, 2013

Sorry for the delay. I will rename it and send another pull request.

@thedustinsmith
Copy link
Contributor

Don't worry about it. It's already in master. Thanks for the help!

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