Skip to content

Introduce Lotus::Controller::Error #135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2015
Merged

Introduce Lotus::Controller::Error #135

merged 1 commit into from
Dec 18, 2015

Conversation

@karimmtarek
Copy link
Contributor

@karimmtarek karimmtarek commented Dec 18, 2015

  • Added Error class
  • Replaced ::StandardError with Lotus::Controller::Error
  • Added a test

jodosha
jodosha reviewed Dec 18, 2015
View reviewed changes
README.md Outdated
Copy link
Member

@jodosha jodosha Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karimmtarek This is unrelated to the scope of your change, please revert it. 😄

Copy link
Contributor Author

@karimmtarek karimmtarek Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodosha Sure I'll revert to the previous readme file :)

@jodosha jodosha self-assigned this Dec 18, 2015
@jodosha jodosha added this to the v0.5.0 milestone Dec 18, 2015
@jodosha
Copy link
Member

jodosha commented Dec 18, 2015

@karimmtarek Hey thanks for this PR. 😄

You replaced all the occurrences of StandardError in the project, but this is unrelated to the scope of your change. With that tests and fixtures we're verifying the behavior of the framework when an exception that comes from the application.

This commit is to revert entirely. 😞

The only change that you should made is for Lotus::Controller::UnknownFormatError that should inherit from Lotus::Controller::Error instead of StandardError.

Can you please fix it? Thanks!

@karimmtarek karimmtarek force-pushed the introduce-lotus-controller-error branch from d72da50 to e51256d Compare December 18, 2015 16:28
@karimmtarek
Copy link
Contributor Author

karimmtarek commented Dec 18, 2015

@jodosha Sorry for missing things up 😞
I'll revert, and see how it goes.

@karimmtarek karimmtarek force-pushed the introduce-lotus-controller-error branch from e51256d to 7f0dcc8 Compare December 18, 2015 16:46
@karimmtarek
Copy link
Contributor Author

karimmtarek commented Dec 18, 2015

@jodosha what about now?

jodosha
jodosha reviewed Dec 18, 2015
View reviewed changes
test/error_test.rb
Copy link
Member

@jodosha jodosha Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karimmtarek It lacks of test case for Lotus::Controller::UnknownFormatError 😉

Copy link
Contributor Author

@karimmtarek karimmtarek Dec 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jodosha Done 👍

@karimmtarek
Introduce Lotus::Controller::Error
c0cb796 
- Added Error class
- Replaced ::StandardError with Lotus::Controller::Error
- Added a test
@karimmtarek karimmtarek force-pushed the introduce-lotus-controller-error branch from 7f0dcc8 to c0cb796 Compare December 18, 2015 17:11
@jodosha
Copy link
Member

jodosha commented Dec 18, 2015

@karimmtarek It looks good now. Thank you! 👍

jodosha added a commit that referenced this pull request Dec 18, 2015
@jodosha
Merge pull request #135 from karimmtarek/introduce-lotus-controller-e…
c6be6c1 
…rror

Introduce Lotus::Controller::Error
@jodosha jodosha merged commit c6be6c1 into hanami:master Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jodosha jodosha

Labels

enhancement

Projects

None yet

Milestone

v0.5.0

Development

Successfully merging this pull request may close these issues.

2 participants

@karimmtarek @jodosha