-
Notifications
You must be signed in to change notification settings - Fork 4
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
Implement support for using a JSON file as request body. #43
Conversation
- Refactor zum to accept a filepath to the contents of the body of a request through a command line argument. - Extract logic to check if the file path provided is valid into a separate function.
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.
Hi! First of all, thank you again for contributing! I really appreciate it, and I have really enjoyed reviewing your PR. As a general working piece of code, I think that it works marvelously, but I will suggest several (mainly structural) changes to improve testability and room for improvement. I also think that each new method should be individually tested, to be sure about exactly what got broken on new features.
if len(arguments) >= 1: | ||
body_file: Path = Path(arguments.pop()) | ||
if body_file.exists() and body_file.is_file(): | ||
with body_file.open("r") as body_contents: | ||
return load(body_contents) | ||
|
||
else: | ||
if not body_file.exists(): | ||
raise InvalidRequestBodyFileError( | ||
f"Request body file located at '{body_file.absolute()}' does" | ||
+ " not exist!" | ||
) | ||
|
||
if not body_file.is_file(): | ||
raise InvalidRequestBodyFileError( | ||
f"Request body file located at '{body_file.absolute()}' is not" | ||
+ " a file!" | ||
) | ||
|
||
raise InvalidRequestBodyFileError( | ||
"Please provide the file path to the request body." | ||
) |
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.
I think that this should be written the other way around to remove indentations and branching. For instance, I would write something like:
if not arguments:
raise InvalidRequestBodyFileError(
"Please provide the file path to the request body."
)
body_file = Path(arguments.pop())
if not body_file.exists():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' does "
"not exist!"
)
if not body_file.is_file():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' is not "
"a file!"
)
with body_file.open("r") as raw_body_contents:
return json.load(raw_bady_contents)
That way, you can also remove the Union[X, NoReturn]
return type and just specify the return type (because clearly that method will always return a dictionary or raise an exception).
@@ -2,12 +2,44 @@ | |||
Module for the core requests logic of zum. | |||
""" | |||
|
|||
from typing import Any, Dict, List | |||
from json import load |
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.
For the json
library, just to keep it consistent with the rest of the codebase, I would prefer if you imported just json
and then used the json.load
method ❤️
Parse the JSON request body into a dictionary and return it if the | ||
specified file exists. | ||
""" | ||
if len(arguments) >= 1: |
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.
I think that if arguments:
is a bit more elegant on this situation
if not body_file.exists(): | ||
raise InvalidRequestBodyFileError( | ||
f"Request body file located at '{body_file.absolute()}' does" | ||
+ " not exist!" |
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.
I would loose the +
operator (as the parentheses will concatenate the strings) and leave the space on the upper line, for consistency's sake
body: Dict[str, Any] = dict() | ||
if ( | ||
raw_endpoint.get("body") is not None | ||
and isinstance(raw_endpoint.get("body"), str) | ||
and raw_endpoint.get("body") == "json" | ||
): | ||
body = load_request_body(remaining_arguments) | ||
else: | ||
body, _ = reduce_arguments(raw_endpoint.get("body"), remaining_arguments) |
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.
I think that this whole piece of code could be decoupled and implemented as a method that receives the output of calling raw_endpoint.get("body")
and the remaining_arguments
and returns the corresponding body. That way, this new function could operate all the conditionals and all that stuff then execute the load_request_body
method directly with the first argument (if it exists)
from zum.requests.helpers import reduce_arguments | ||
from zum.requests.models import Request | ||
|
||
|
||
def load_request_body(arguments: List[str]) -> Union[Dict[str, Any], NoReturn]: |
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.
I think that this method could explicitly say file
, so that it is obvious that it retrieves the contents of a file. For example, it could be named load_request_body_from_file
. Also, I think that it should receive only the first element of the arguments
list (pre-processing that information outside this method). I don't know what should happen if arguments
were to be empty, maybe you should pass None
or directly raise the error from outside the method. I leave that to your judgement. Lastly, the return type should be only Dict[str, Any]
. On another comment I left I explained how to stop mypy
from whining about the return type by refactoring the method's code
if not body_file.is_file(): | ||
raise InvalidRequestBodyFileError( | ||
f"Request body file located at '{body_file.absolute()}' is not" | ||
+ " a file!" |
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.
I would loose the +
operator (as the parentheses will concatenate the strings) and leave the space on the upper line, for consistency's sake
Thanks for the review! I will implement all the suggested changes. |
[Feature]: Support for using files as request bodies. May resolve issue #8
Description
This pull request adds support for using the contents of a file as the JSON body of a request. This is currently done by providing a
bodyPath
variable in thezum.toml
file. If the file path exists and contains a file, the contents of the file are read and parsed as JSON. The contents of the file are then sent with the request.If the user provides both a
body
andbodyPath
variable like:the contents of
body
are ignored and the file located atbodyPath
is used instead.Requirements
None.
Additional changes
Added tests for the new functionality.