-
Notifications
You must be signed in to change notification settings - Fork 6
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
Changing logic to return payload as json in the case of any exception depending on received bytes #20
Conversation
… depending on recevied bytes
except socket.error as e: | ||
self.logger.error(f"ClientSocket SocketError: {e}") | ||
raise socket.error(f"ClientSocket SocketError: {e}") |
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.
if we are removing all the raise exceptions
, then why don't we consolidate all these exceptions into a single one and just log the error
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 could potentially do that, only downside is that we would lose how long a socket timeout took. If you think it is worth it to simplify the code, then I can go ahead and do that.
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.
Except socket.timeout, you can bundle rest of the exceptions to a generic exception unless it has some value.
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.
sounds good, thank you
except ValueError as e: | ||
raise ValueError(f"ServerSocket: ValueError - {e}") |
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.
the top comment applies here as well
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.
responded to comment above
…nd ServerSocket classes
Kudos, SonarCloud Quality Gate passed! |
tested on 4 node vm cluster successfully.