I am working on an API wrapper for calls to a website. Each of several types of calls can take a large number of optional arguments, some of which contradict. In the interest of preventing the user from supplying incompatible parameters, I would like to verify that the arguments are correct before submitting the request.
However, the simplest technique to do so has resulted in the below nightmare, with several if-statements. I know this is not a best practice, but I am unsure what the best method to correct it would be. I know that using *kwargs would probably be involved, but I don't see how that would save me from verifying the arguments in much the same way as below. I have heard that I should probably wrap the arguments in a class, but I'm unsure how much of a better idea that is.
def list_tag_alias(
self,
name_matches: str = None,
status: str = None,
order: str = None,
antecedent_tag: int = None,
consequent_tag: int = None,
limit: int = None,
before_page: int = None,
after_page: int = None,
page: int = None,
):
query_url = "tag_aliases.json?"
arguments = []
if name_matches != None:
arguments.append("search[name_matches]={}".format(name_matches))
if status != None:
assert status.lower() in (
"approved",
"active",
"pending",
"deleted",
"retired",
"processing",
"queued",
"blank",
), "status must be one of: approved, active, pending, deleted, retired, processing, queued, or blank."
arguments.append("search[status]={}".format(status.lower()))
if order != None:
assert order.lower() in (
"status",
"created_at",
"updated_at",
"name",
"tag_count",
), "order must be one of status, created_at, updated_at, name, tag_count"
arguments.append("search[order]={}".format(order.lower()))
if antecedent_tag != None:
assert 0 <= antecedent_tag <= 8
arguments.append(
"search[antecedent_tag][category]={}".format(str(antecedent_tag))
)
if consequent_tag != None:
assert 0 <= consequent_tag <= 8
arguments.append(
"search[consequent_tag][category]={}".format(str(consequent_tag))
)
if limit != None:
assert 0 <= limit <= 1000, "limit must be between 0 and 1000, inclusive"
arguments.append("limit={}".format(str(limit)))
if before_page != None:
assert (
after_page == None and page == None
), "only one of before_page, after_page, or page must be supplied."
assert before_page >= 0, "before_page must be greater than 0"
arguments.append("page=b{}".format(str(before_page)))
if after_page != None:
assert (
before_page == None and page == None
), "only one of before_page, after_page, or page must be supplied."
assert after_page >= 0, "after_page must be greater than 0"
arguments.append("page=a{}".format(str(after_page)))
if page != None:
assert (
before_page == None and after_page == None
), "only one of before_page, after_page, or page must be supplied."
arguments.append("page={}".format(str(page)))
query_url += "&".join(arguments)
request = WebRequest(state_info=self.state_info)
response = request.submit_request(query_url=query_url)
return response.json()
```
1 Answer 1
Don't assert; raise exceptions
assert statements may be disabled by flag (python -O ...) or environment variable.
Use raise ValueError("status must be one of: approved...") instead.
Don't test for equality with None
None is a singleton value. You should use status is not None instead of status != None.
You can just test the value directly, None is a false value. So if status: can be used instead of if status != None: or if status is not None:. Note: that other things test as false, such as 0, "", and [], so this test is not exactly the same.
Use enums
Strings are hard to use. Should you pass "delete", "deleted", or "deleting", ... or maybe "removed"?
Enumerations give you named constants to use. Your IDE may even help you autocomplete the constant name.
from enum import Enum
class Status(Enum):
APPROVED = "approved"
ACTIVE = "active"
...
...
if status:
arguments.append("search[status]={}".format(status.value))
You can turn strings into enums like:
if status is not None and not isinstance(status, Status):
status = Status(status.lower())
So the caller could pass in "deleted" or "DELETED" or Status.DELETED and they would all work, but if they passed in "removed", you'd get a ValueError: 'removed' is not a valid Status exception. So you no longer have to manually test if the given status is a legal status word. (However, if not every status word is legal in a function, these would still need to be tested.)
Common functions
If you have several functions which take before_page, after_page and page arguments, you probably have the same validation requirements in each. You should call another method to validate these, something like:
self._validate_pages(before_page, after_page, page, arguments)
Positional & keyword-only arguments
As it stands, a caller may try call obj.list_tag_alias(None, None, "approved") to list the approved tag aliases. But actually, that used "approved" as the order parameter; while status is the 3th argument, that includes self in the count.
The correct call would be either obj.list_tag_alias(None, "approved"), or better, obj.list_tag_alias(status="approved").
You can deny the positional argument variant by forcing the parameters to be keyword-only parameters, by adding a * to the argument list:
def list_tag_alias(
self,
name_matches: str = None,
*, # remaining parameters are keyword-only
status: str = None,
order: str = None,
...
Now you can call obj.list_tag_alias("fred") or obj.list_tag_alias(status="approved"), but obj.list_tag_alias(None, "approved") is an error:
TypeError: list_tag_alias() takes 1 positional argument but 2 were given
Argument dictionary
arguments = []
...
arguments.append("search[name_matches]={}".format(name_matches))
...
query_url += "&".join(arguments)
You are using "...={}".format(...) and .append() multiple times to create your argument list to produce the query string. Consider using a dictionary instead:
arguments = {}
...
arguments["search[name_matches]"] = name_matches
...
query_url += "&".join(f"{key}={value}" for key, value in arguments.items())
Bonus: using f"...{value}" is interpolating the value into a string, so str() calls for limit, tag, and page parameters are unnecessary.