Skip to content
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

Panic less and with better messages #281

Merged
merged 2 commits into from
Oct 20, 2018
Merged

Panic less and with better messages #281

merged 2 commits into from
Oct 20, 2018

Conversation

trinity-1686a
Copy link
Contributor

Fix #21
Change most unwrap to expect, and don't panic when it can be avoided. Also return better http codes, i.e. 400 and 404, when appropriate.
The trait rocker::request::FromParam should probably be implemented for Post, Blog, Media, User and Tag. Then we wouldn't need to return Options whenever an entity does not exist, and just let Rocket do the work for us

Change all unwrap to expect
Normalize expect's messages
Don't panic where it could be avoided easily
@trinity-1686a trinity-1686a added C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed A: Backend Code running on the server labels Oct 20, 2018
Copy link
Member

@elegaanz elegaanz left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@elegaanz
Copy link
Member

And yes, implementing FromParams for these types would be a good idea.

@elegaanz elegaanz merged commit 879fde8 into master Oct 20, 2018
@trinity-1686a trinity-1686a deleted the panic-refactor branch October 20, 2018 10:48
@trinity-1686a
Copy link
Contributor Author

After trying to, I guess it might not be possible, because we need a db connection, and I don't think there is a way to access managed state from from_param (it only takes a &RawStr as parameters). Nonetheless I'll ask on Matrix

@elegaanz
Copy link
Member

Maybe with with FromRequest and Request::get_param then? But it may force us to duplicate parameters in the routes signatures…

@trinity-1686a
Copy link
Contributor Author

If we end up doing manual routing it may reduce code duplication a bit, but it will add a lot of complexity, and we won't be able to use rocket's code generation if we do so. I'll see what answer I get from Matrix, but what I said earlier might very well be a false good idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Backend Code running on the server C: Enhancement New feature or request S: Ready for review This PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants