search

onyxframework / http

An opinionated framework for scalable web 🌎
https://onyxframework.com/http
MIT License
143 stars 11 forks source link

use abstract superclass for param errors #46

Closed repomaa closed 5 years ago

repomaa commented 6 years ago

this allows type safe rescuing

vladfaust commented 6 years ago

So, what's the main purpose on this PR? To introduce abstract ParamError class? I agree on that, however the code seems to become much more difficult.

repomaa commented 6 years ago

Hm yeah, it was an attempt to DRY things up a little. I can revert back to the way things were before if you like.

vladfaust commented 6 years ago

Yes, please keep the things simpler. Also I'd like you to conform with Angular Commit Styling (as seen at https://github.com/vladfaust/prism/commits/master). I suggest the commit message to be as following:


feat: abstract `ParamError` class

`InvalidParamTypeError`, `ParamNotFoundError`, `InvalidParamError` and `ProcError` all now extend `ParamError` class
repomaa commented 6 years ago

will do!

repomaa commented 6 years ago

Better?

vladfaust commented 6 years ago

Yes, that's better, but still not ideal. I also would like to change how Action::Params handle errors. Could you please give me an access to your PR? https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/

repomaa commented 6 years ago

allow edits from maintainers is already checked.

repomaa commented 6 years ago

I left the detail param to be String? because of the off chance that someone would call the initializer and pass nil to it so nothing would break. I don't like it though. The default could be passed to @detail : String directly in the params. @message was a bit of an ill choice for a instance var for a class that inherits from Exception because it gets overwritten in the Exception constructor ;)

vladfaust commented 6 years ago

You'd also want to close #19

repomaa commented 6 years ago

Close #19? How come? I don't deal with any error codes here.

vladfaust commented 6 years ago

If Action::Params is changed, #19 can be closed alongside

repomaa commented 6 years ago

I think that should be a separate PR since it's a completely separate issue.