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

Return behavior of functions should be normalized #1445

Open
kamakazikamikaze opened this issue Aug 23, 2024 · 0 comments
Open

Return behavior of functions should be normalized #1445

kamakazikamikaze opened this issue Aug 23, 2024 · 0 comments

Comments

@kamakazikamikaze
Copy link

The Confluence class has several functions documented to return a specific type. However, when a target page does not exist the function will log an error and return None. This behavior is not always documented for the user to expect/check.

Confluence.get_page_by_title will intercept an error and return None, which is documented for the user.

def get_page_by_title(self, space, title, start=0, limit=1, expand=None, type="page"):
"""
Returns the first page on a piece of Content.
:param space: Space key
:param title: Title of the page
:param start: OPTIONAL: The start point of the collection to return. Default: None (0).
:param limit: OPTIONAL: The limit of the number of labels to return, this may be restricted by
fixed system limits. Default: 1.
:param expand: OPTIONAL: expand e.g. history
:param type: OPTIONAL: Type of content: Page or Blogpost. Defaults to page
:return: The JSON data returned from searched results the content endpoint, or the results of the
callback. Will raise requests.HTTPError on bad input, potentially.
If it has IndexError then return the None.
"""

try:
return response.get("results")[0]
except (IndexError, TypeError) as e:
log.error("Can't find '%s' page on the %s!", title, self.url)
log.debug(e)
return None

Confluence.get_tables_from_page will return a dictionary with a completely different key structure which isn't documented. Either the same structure should be followed with "number_of_tables_in_page" set to 0 or an exception should be raised.

if len(tables_raw) > 0:
return json.dumps(
{
"page_id": page_id,
"number_of_tables_in_page": len(tables_raw),
"tables_content": tables_raw,
}
)
else:
return {
"No tables found for page: ": page_id,
}

Confluence.attach_content will return the API response upon success or None when a target page does not exist. This is not documented.

return response
else:
log.warning("No 'page_id' found, not uploading attachments")
return None

Confluence.download_attachments_from_page returns a str if no attachments are found, otherwise it returns a dict.

Confluence.update_page will return None if page does not exist. This is not documented for the user.

except (IndexError, TypeError) as e:
log.error("Can't find '%s' %s!", title, type)
log.debug(e)
return None

These are just a small sample of functions where behavior diverges from the documentation or unnecessarily changes the return type. I would suggest either:

  • Raising an exception when a target page does not exist (similar to an ApiNotFoundError, but as a new unique exception). However this would break backwards-compatibility for existing users.
  • Correcting function documentation to warn of the varying return types and/or normalizing structure of certain returns (e.g. dict for Confluence.get_tables_from_page using same keys).
@kamakazikamikaze kamakazikamikaze changed the title Return behavior of functions should be normalized to establish expected behavior Return behavior of functions should be normalized Aug 23, 2024
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

No branches or pull requests

1 participant