search

coderberry / airbrake-grails

Airbrake Client for Grails
4 stars 8 forks source link

resolved #19 and #20 #22

Closed ghost closed 11 years ago

ghost commented 11 years ago

Here's my 2nd attempt at #19 and #20. I've taken into consideration the comments you made on my previous submission

jonspalmer commented 11 years ago

There are a bunch of things going on in this pull request. #19, #20, an upgrade to 2.1.1 and some extra logging. It would me much easier to submit separate pull requests that can be considered independently. There are some questions still around the thread pooling solution that prevent me from accepting the other much more straight forward changes.

Also a general practice for many open source projects is NOT to include version changes in pull requests. Its normally much easier for the project committers to increment version numbers as they package a release. I'd suggest we follow that pattern here.

ghost commented 11 years ago

I'd be happy to code a solution of that myself

I agree that it makes sense to integrate this with the existing async closure handling if that's possible (I haven't really looked at how this works, to be honest). If you're offering to do this, I'm happy to accept

ghost commented 11 years ago

Also a general practice for many open source projects is NOT to include version changes in pull requests. Its normally much easier for the project committers to increment version numbers as they package a release. I'd suggest we follow that pattern here.

That seems like a sensible practice, I'll revert the project version change and the Grails upgrade

ghost commented 11 years ago

Further to your comments regarding this pull request, I've made the following changes:

I think that's everything apart from integrating the thread pool with the existing custom asynch handling, which I think you offered to do yourself.

jonspalmer commented 11 years ago

I've merged this request and then reworked it a little to put the Thread pooling logic entirely in the Configuraiton. It makes testing a whole lot easier and I was able to get good specs around most of the relevant changes.