Return Synapse client error instead of ValueError where applicable
Background: Consider the following instruction where `team_name` is the name of a team that already exists on Synapse.
syn.store(synapseclient.Team(name=team_name, description=desc, canPublicJoin=can_public_join))
Issue: The above instruction raises an exception of type `ValueError`.
1. See https://github.com/Sage-Bionetworks/synapsePythonClient/blob/4047cd179df5d965049ab1df938c51295096aa95/synapseclient/client.py#L920
2. See https://github.com/Sage-Bionetworks/synapsePythonClient/blob/3b3155aa3c1aaa81ee561724e991d3caca8cf58e/synapseclient/dict_object.py#L17
Suggested solution: It would be great to return a `SynapseHTTPError` object to the user of the python client so that he/she can handle different scenarios based on the value of a status code documented by Synapse (e.g. status code of the rest api response).
, I totally agree with with raising an SynapseError. The description mentioned SynapseHTTPError, which is an error that comes from Synapse backend. In the case that the backend throws an error, we should returns a SynapseHTTPError. In the case that “the logic of the python client to not accept the creation of team from a name that is already used,” we should returns a client error.
Thank you for suggesting the change! I am going to change the title of the ticket to remind myself what this ticket is about. Please feel free to comment if it no longer describes what you asked for.
can you include the current error?
I am using the synapse python client programmatically. The error is not relevant here. An exception ValueError is raised when the line of code previously mentioned is executed, which is expected (see Background section of the ticket).
Also, why would you prefer to use SynapseHTTPError instead of ValueError?
It is a good design practice to encapsulate the exceptions of a package into custom exception like the ones that already exist such as SynapseHTTPError, SynapseFileCacheError and the generic SynapseError. You already have that in place and this is great! In my program, when I execute the following line of code:
, which is a method provided by the synapse python client, an ValueError is raised. The reason I suggest to raise an exception that implement SynapseError is because the exception raised is the result of the logic of the python client to not accept the creation of team from a name that is already used. Another logic could have been to return the handle to the team if the user who execute the program is the owner of the team (note that this logic is actually implemented when creating a Project, i.e. a handle to the Project is return to the user if the Project exists and if the user is is the owner/has edit access to the project).
Therefore, it is fine if the python client raise ValueError if this is an unexpected result. However, if we provide the user with a method to create a Team, the case where the user requests the creation of a Team with a name that is already being used is expected and thus the exception raised should implement/extend SynapseError.
Hi , can you include the current error?
Also, why would you prefer to use SynapseHTTPError instead of ValueError? Was there a best practice where HTTP error is prefer over ValueError in implementing a RESTful client? (independent to the current implementation of synapseclient)