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

Exception handling request hook #253

Merged
merged 4 commits into from Sep 30, 2011

Conversation

Projects
None yet
3 participants
@chrisnicola

chrisnicola commented Aug 10, 2011

I hope you guys like this. I've been using it for quite a while actually to allow for flexible handling of specific exceptions as well as general exception handling. I've hooked it up in such a way that it falls back to the default behavior of sending an InternalServerError when there is no hook, or the hook itself throws an exception or fails to set a response.

@sloncho

This comment has been minimized.

Show comment
Hide comment
@sloncho

sloncho Sep 22, 2011

Contributor

What's the status of this one? I like the implementation and I would like to use it, but I do not want to diverge too much from the mainstream, so is there any idication if this is going to be included in 0.8 or not?

Contributor

sloncho commented Sep 22, 2011

What's the status of this one? I like the implementation and I would like to use it, but I do not want to diverge too much from the mainstream, so is there any idication if this is going to be included in 0.8 or not?

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Sep 22, 2011

Github needs voting for pull requests ;-). Hey @thecodejunkie can you take a quick look at this. There isn't much too it and it's been in production for a bit now.

chrisnicola commented Sep 22, 2011

Github needs voting for pull requests ;-). Hey @thecodejunkie can you take a quick look at this. There isn't much too it and it's been in production for a bit now.

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev Sep 23, 2011

Member

@LucisFerre @thecodejunkie I like the idea in principal, but there's a few issues:

  • We have made a few changes to the pipelines so they can support named items - it would (probably) make sense to keep parity with those and this new exception pipeline - this also means it's not a clean merge.
  • There are some "rogue" edits in the diff that seem to come from tab/spacing? Not the end of the world, but it's a bit messy :-)
  • The code doesn't fit our Swedish task master's coding style ( if (this.OnErrorHook == null) throw ex; for instance )
  • I'm not 100% sure on the "one catch to rule them all", rather than individual catches around the request lifetime in the engine - although I think we can live with that for now.

So, I know it's taken forever to get this looked at, and my apologies for that, but if you can rebase and rejigger a few bits of it we should be able to get it in for 0.8 :-)

Member

grumpydev commented Sep 23, 2011

@LucisFerre @thecodejunkie I like the idea in principal, but there's a few issues:

  • We have made a few changes to the pipelines so they can support named items - it would (probably) make sense to keep parity with those and this new exception pipeline - this also means it's not a clean merge.
  • There are some "rogue" edits in the diff that seem to come from tab/spacing? Not the end of the world, but it's a bit messy :-)
  • The code doesn't fit our Swedish task master's coding style ( if (this.OnErrorHook == null) throw ex; for instance )
  • I'm not 100% sure on the "one catch to rule them all", rather than individual catches around the request lifetime in the engine - although I think we can live with that for now.

So, I know it's taken forever to get this looked at, and my apologies for that, but if you can rebase and rejigger a few bits of it we should be able to get it in for 0.8 :-)

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Sep 23, 2011

It's easy for me to run a quick resharper cleanup. Would make a big difference if you guys included a resharper settings file with the project though.

chrisnicola commented Sep 23, 2011

It's easy for me to run a quick resharper cleanup. Would make a big difference if you guys included a resharper settings file with the project though.

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev Sep 26, 2011

Member

@LucisFerre it's pretty much StyleCop rules, although there is an r# file in one of the open pull requests. It's just good OSS contributor etiquette to try and keep in with the coding style (even if you disagree with it, which I do ;-)) .. the tooling is pretty irrelevant.

Anyway, we're looking for finalise 0.8 for the end of this week, do you think you can get this rebased/rejigged by then?

Member

grumpydev commented Sep 26, 2011

@LucisFerre it's pretty much StyleCop rules, although there is an r# file in one of the open pull requests. It's just good OSS contributor etiquette to try and keep in with the coding style (even if you disagree with it, which I do ;-)) .. the tooling is pretty irrelevant.

Anyway, we're looking for finalise 0.8 for the end of this week, do you think you can get this rebased/rejigged by then?

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Sep 29, 2011

It's not that I'm trying to be disagreeable it's that my IDE is already set up to enforce a certain style so changing it just to suit one project isn't practical. If there isn't an included style file to override it this is going to regularly be a problem. I'll do my best though.

chrisnicola commented Sep 29, 2011

It's not that I'm trying to be disagreeable it's that my IDE is already set up to enforce a certain style so changing it just to suit one project isn't practical. If there isn't an included style file to override it this is going to regularly be a problem. I'll do my best though.

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev Sep 29, 2011

Member

@LucisFerre there is definitely an R# style file in one of the pull requests, I've never used it though. I would like to get this into 0.8, as it's definitely of value, so if you don't think you'll be able to do it tonight (UK time) can you let me know and I'll pull it in locally as is and fix it up.

Cheers!

Member

grumpydev commented Sep 29, 2011

@LucisFerre there is definitely an R# style file in one of the pull requests, I've never used it though. I would like to get this into 0.8, as it's definitely of value, so if you don't think you'll be able to do it tonight (UK time) can you let me know and I'll pull it in locally as is and fix it up.

Cheers!

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Sep 29, 2011

Yeah so it looks like one of the files has mixed tabs and spaces. I am switching it to spaces which I assume is correct (since it is obviously correct ;-)). There appear to be a few files with this problem though.

chrisnicola commented Sep 29, 2011

Yeah so it looks like one of the files has mixed tabs and spaces. I am switching it to spaces which I assume is correct (since it is obviously correct ;-)). There appear to be a few files with this problem though.

@chrisnicola

This comment has been minimized.

Show comment
Hide comment
@chrisnicola

chrisnicola Sep 29, 2011

Should all be good now. My scroll-wheel is complaining a bit but other than that it looks consistent if a bit verbose.

chrisnicola commented Sep 29, 2011

Should all be good now. My scroll-wheel is complaining a bit but other than that it looks consistent if a bit verbose.

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev Sep 29, 2011

Member

Thanks.. the mixedbtabs/spaces stuff îs probably files we've tweaked for mono in monodevelop.. its always moaning about stuff like that.

Member

grumpydev commented Sep 29, 2011

Thanks.. the mixedbtabs/spaces stuff îs probably files we've tweaked for mono in monodevelop.. its always moaning about stuff like that.

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev Sep 30, 2011

Member

Thanks - just had a look and it's still not using our new pipeline stuff - I'll check out locally this morning and switch it over - should mainly be removing code.. which is good :-)

Member

grumpydev commented Sep 30, 2011

Thanks - just had a look and it's still not using our new pipeline stuff - I'll check out locally this morning and switch it over - should mainly be removing code.. which is good :-)

@grumpydev grumpydev merged commit 90550d5 into NancyFx:master Sep 30, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment