-
-
Notifications
You must be signed in to change notification settings - Fork 19
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
Notify handle #45
Notify handle #45
Conversation
Merde. I try to look what has changed in the test series. |
Ok now the tests pass with the new test framework. |
Hi, sorry for the late reply on this. I've been swamped with other work and also been changing $DAYJOB, so haven't had much time or energy to go through all PRs and issues on GitHub until now. Also, as you mention, we have a bit of history from before and I've honestly been holding off reviewing this much because of that. Now, I've checked out this your initial branch, looked over the code changes and test. Besides from the actual review (coming up), I have the following general feedback:
I'll do a detailed review now, using the GitHub PR review. |
Hello, thanks for looking. |
I will look into this next week, ok? |
Considering how long it took me to respond, of course! :) That also gives me time to clean up the test code a bit. Got quite a ways tonight, will continue tomorrow. |
Thanks again for your interest in adding this. |
OK, I'll have a look later, but you''re probably right. |
Funnily exactly 2 years after my private-mail attempt, now also on github (though i do not like it, just to lessen burden on people like you), here is the notify script patch again.
Note i have split the test out into a separate commit, i have not tried the test ever since.
The code i use ever since, too.