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

feat: handle fxcoded exception and return readable response #164

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

tehreem-sadat
Copy link
Collaborator

Related Issue: #161

Copy link
Collaborator

@shadinaif shadinaif left a comment

Choose a reason for hiding this comment

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

Thank you @tehreem-sadat , this is great!

I have one suggestion regarding the stand-alone function. On the other hand, if you're not comfortable with having the code inside FXViewRoleInfoMixin, then you can create another mixing that FXViewRoleInfoMixin inherits. So, anything that inherits FXViewRoleInfoMixin will have the new mixing automatically. I'll leave it to you to decide

Comment on lines 68 to 75
def fx_exception_handler(exc: Exception) -> Response | None:
"""Handle exception and return response with error details"""
if isinstance(exc, FXCodedException):
return Response(
error_details_to_dictionary(reason=str(exc)),
status=status.HTTP_400_BAD_REQUEST
)
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need for a stand-alone function in my opinion. Please move the code to handle_exception method

@tehreem-sadat tehreem-sadat force-pushed the tehreem/handle_fxcoded_exception branch from 4153873 to fc6f92e Compare December 10, 2024 08:19
@tehreem-sadat tehreem-sadat merged commit 05a3c4d into main Dec 10, 2024
3 checks passed
@tehreem-sadat tehreem-sadat deleted the tehreem/handle_fxcoded_exception branch December 10, 2024 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants