Add some type hints and docstrings for app.py #677
Conversation
|
Not sure if this should target master or devel? |
|
Hey @yuvipanda, thanks for the PR! The way we have the repo set up right now, we make PR's into |
I loved reading https://github.com/asfadmin/thin-egress-app/blob/master/docs/vision.md! Hopefully this PR is useful and not just noise. I couldn't figure out how exactly to type a return for the S3 botocore client, but otherwise I think this is accurate.
yuvipanda
force-pushed
the
some-type-hints
branch
from
8753952 to
502710c
Compare
|
Great! git macigck'd and base changed :) |
yuvipanda
changed the title
|
@mckadesorensen sorry to change the PR after you had already reviewed it! I added some more docstrings as I read through the code to help me understand what was going on :) Hope that is useful too! |
|
Also lmk what else I can do to help get this merged! |
There was a problem hiding this comment.
There's 1 line that becomes too long when adding the return type annotation. We'll need to fix that before being able to merge (see github actions).
Technically our coding standards say that docstrings should follow this format:
"""A multi line docstring has a one-line summary, less than 120 characters.
Then a new paragraph after a newline that explains in more detail any
general information about the function, class or method. Example usages
are also great to have here if it is a complex class or function.
When writing the docstring for a class, an extra line should be placed
after the closing quotations. For more in-depth explanations for these
decisions see <http://www.python.org/dev/peps/pep-0257/>
If you are going to describe parameters and return values, use Sphinx, the
appropriate syntax is as follows.
:param foo: the foo parameter
:param bar: the bar parameter
:returns: return_type -- description of the return value
:returns: description of the return value
:raises: AttributeError, KeyError
"""
This is straight from the OpenStack style guide.
| # If opentelemetry is not installed, we make dummy objects / functions here | ||
| # so we do not need to add conditionls throughout our codebase |
There was a problem hiding this comment.
Personally, I prefer to err on the side of having fewer comments because of the risk of them becoming out of date and straight up wrong over time. To me the 'except ImportError and replace with None' pattern seems standard enough that it doesn't warrant a comment. What do others think in this case?
There was a problem hiding this comment.
I agree with @reweeden on this comment. I have been burned too many times by people not updating comments when they update code.
There was a problem hiding this comment.
My personal experience has been that the more common phenomena is lack of comments rather than wrong comments, and usually this 'mismatch' issue is dealt with during code review of the changing code itself rather than by not having comments at all.
And as for this specific pattern, I've personally seen a different pattern (detailed in https://adamj.eu/tech/2021/12/29/python-type-hints-optional-imports/) be used more often. It also has the advantage of working better with mypy and other type checkers, rather than simply setting to None. This was the primary reason I added the comment here.
However, y'all are the maintainers so are ultimately responsible for the state of the code. So I'll happily remove the comment if you think that's what's best for this project.
Thanks for your consideration!
There was a problem hiding this comment.
Unfortunately I don't think code reviews can be relied upon to catch comment mismatches. Stuff like that slips through unnoticed all the time which is the nature of the problem.
I'd say, lets go for no comment in this case.
| assert app.lambda_context is not None | ||
|
|
||
| return app.lambda_context.aws_request_id | ||
|
|
||
|
|
||
| @with_trace() | ||
| def get_origin_request_id() -> Optional[str]: | ||
| """ | ||
| Return the *original* AWS Lambda Request ID |
There was a problem hiding this comment.
I'm actually not sure what x-origin-request-id is. But I believe 'origin' has a specific meaning in HTTP and I think AWS is using it in a similar way.
| @@ -367,12 +420,18 @@ def get_user_from_token(token): | |||
|
|
|||
| @with_trace() | |||
| def cumulus_log_message(outcome: str, code: int, http_method: str, k_v: dict): | |||
| """ | |||
| Emit log message to stdout in a format that cumulus understands | |||
There was a problem hiding this comment.
Does anyone know if this actually interacts with cumulus at all?
There was a problem hiding this comment.
@benbart Might know the answer to that.
There was a problem hiding this comment.
More like cumulus interacts with the log message generated. This outputs the log message in a format that cumulus likes, if memory serves. The Issue #275 isn't very descriptive on this matter.
There was a problem hiding this comment.
My understanding would be that there's some elasticsearch / kibana rule that is looking for log messages in this format to expose specially, and this is writing things out in that format. If you think a different kinda comment would be more clarifying here, I can do that.
|
Thank you for the comments! I'm going to try to come back to this whenever I can, and do bits and pieces whenever I get the time :) |
|
@mckadesorensen re: docstrings, my hope is that adding some is better than none. I often find it difficult to write param dosctrings, as they often are repetitive. However, if you would like me to make sure all docstrings are 'complete', I can try (although would prefer not to haha). |
|
Params in docstrings are optional. Just format what you have so it matches the example and that will be fine. |
I loved reading
https://github.com/asfadmin/thin-egress-app/blob/master/docs/vision.md! Hopefully this PR is useful and not just noise.
I couldn't figure out how exactly to type a return for the S3 botocore client, but otherwise
I think this is accurate.
what it's doing.