-
Notifications
You must be signed in to change notification settings - Fork 0
94 back end add exception handling and logging #100
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
base: master
Are you sure you want to change the base?
94 back end add exception handling and logging #100
Conversation
erfanul007
commented
Jun 17, 2023
- Added configuration of logging
- Updated all methods with try catch
- Server error logging added
…ging # Conflicts: # suggestion_app/services/generateSuggestion.py
smhimran
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. A few improvements can be made, some of them are not urgent and we can do sometime in the (not so soon ;p) future.
| ports: | ||
| - "80:80" | ||
| - "8080:80" | ||
| volumes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary? Nginx should run on docker port 80 without any errors.
| return Response({'message': 'User not found'} ,status=404) | ||
| except Exception as ex: | ||
| logger.exception(ex) | ||
| return Response(status=500, data={'message': 'Some error occurred while getting user score.'}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, you can raise the exception without having to return a 500 error. However, that's optional.
|
|
||
| if response1.status_code != 200 or response1.json()['status'] != 'OK': | ||
| try: | ||
| response1 = requests.get("https://codeforces.com/api/user.rating?handle=" + cf_username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should move these API requests to a different service...as an improvement of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erfanul007 Add a todo and an issue to move this API call and other API calls to a service. We can close the PR.