docs: Add docstrings to Seam class #115
Conversation
seam/seam.py
Outdated
| :raises SeamHttpApiError: For general API errors, including | ||
| unexpected server responses | ||
| :raises SeamHttpUnauthorizedError: When the provided authentication | ||
| credentials (api_key or personal_access_token) are invalid | ||
| :raises SeamHttpInvalidInputError: When the API request contains | ||
| invalid input data | ||
| :raises SeamActionAttemptFailedError: When an action attempt fails to | ||
| complete successfully (only when wait_for_action_attempt is | ||
| enabled) | ||
| :raises SeamActionAttemptTimeoutError: When an action attempt exceeds | ||
| the specified timeout duration (only when wait_for_action_attempt | ||
| is enabled) |
There was a problem hiding this comment.
These are all raised by the individual methods. Does it make sense to put them on the constructor? I wonder what other libs do, since often errors are raised in many places.
There was a problem hiding this comment.
Oh yeah I meant to ask about this but forgot. Probably it doesn't make sense to put them all here. Http errors can go to SeamHttpClient docstring and action attempt ones to the ActionAttempts route class docstring. I think SeamInvalidOptionsError and SeamInvalidTokenError is fine to leave here.
I wonder what other libs do, since often errors are raised in many places.
I briefly looked through a couple of libs and it seems that mostly exceptions are documented where they're raised. I also found instances where exception docstring is also propagated up where the method throwing it is used like here.
Even though InvalidOptionsError is raised in get_auth_headers which in turn is called in parse_options that we use in the Seam constructor, I think it still makes sense to document it on the constructor because none of those functions handle the error and it'll get raised when the constructor is called. Plus if the error is not documented on the constructor, it won't be clear from the IDE intellisense that when calling Seam constructor such errors can be raised. Ideally we should document the error on each level where it can be raised?
|
@DebbieAtSeam Do you want to review the language we use in the docstrings here? See also https://github.com/seamapi/python/pull/113/files#diff-6aa596a1d9b2ede6d6729f736e0d98900fee277a5b3f129509f9485f4ba86d72 |
|
@razor-x Yes, I'd like to do a light copy edit, please. Thanks! |
|
@razor-x Further, are the items with new docstrings here things that do not exist (and could not be documented once) in a centralized location that could populate to all the SDKs? |
There will be similarity across SDKs but it will be language specific. I'd say we should limit the content as much as possible and link to the docs if we need to expand. So, even this sentence feels mostly extraneous
|
|
@DebbieAtSeam @razor-x, are we good to merge this? Or the docstring language is still to be reviewed? |
|
Let's merge it now and then think about editing it and syncing the wording with the JS version later on. |
wrt #86