Fix mypy warnings in router.py #1038
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
CodSpeed Performance ReportMerging #1038 will not alter performanceComparing Summary
|
| description2, headers2, status_code2 = res | ||
| description2 = self._format_response(description2).description | ||
| new_headers: Headers = Headers(headers2) | ||
| if new_headers.contains("Content-Type"): | ||
| headers2.set("Content-Type", new_headers.get("Content-Type")) |
There was a problem hiding this comment.
Hey @dave42w 👋
Thank you for the PR :D
One question -
why are we using description2 or headers2 here? Is it something that mypy suggests?
There was a problem hiding this comment.
They just have to be different names to the ones earlier in _format_response. mypy objects to reusing a variable (even if it has gone out of scope) with a different type. eg this is unacceptable
if x:
y: int = 5
else:
y: str = "five"
There was a problem hiding this comment.
@sansyrox are we ok to consider this point resolved?
There was a problem hiding this comment.
Hey @dave42w 👋
Apologies for the late review.
They just have to be different names to the ones earlier in _format_response. mypy objects to reusing a variable (even if it has gone out of scope) with a different type. eg this is unacceptable
We can disable mypy for these lines then.
mypy objects to reusing a variable (even if it has gone out of scope) with a different type
This is not a good reason IMO. If the code makes sense in our repo, we should not alter it according to mypy.
There was a problem hiding this comment.
H'mm, it seems risky to me. We are deep in lots of nesting at this point. It would be easy for a code change to end up trying to use these variables later in the function (especially because we don't return early) and depending on the data have 2 different types.
I would like to follow this with a more significant refactor anyway to reduce the levels of indentation/nested if statements. At that point this will not be a problem.
There was a problem hiding this comment.
Alright then. but can we get rid of numbers in the variable names. I get a massive ick by that(I know and I am sorry) but something else without involving numbers?
| headers = Headers({}) | ||
| if "Content-Type" not in headers: |
There was a problem hiding this comment.
Hey @dave42w , would this not be false always? I think we have a logical bug in Robyn
There was a problem hiding this comment.
I agree. I was just fixing the mypy first. I think this is ripe for a refactor to clear up the logic, reduce complexity and add unit tests.
Description
This PR fixes most of the mypy warnings in router.py
Summary
This PR does not fix #1035 where add_route is inconsistent between the BaseRouter(ABC) and it's subclasses.
The biggest changes were in Router. _format_response
if "x" in headersreplaced withif headers.contains("x")Beyond that a few places with a missing dict type: for injected_dependencies x 2 and self.reoutes x 1
PR Checklist
Please ensure that:
Pre-Commit Instructions: