Refactoring router to utilize get_session_context#411
Refactoring router to utilize get_session_context#411gialmisi merged 12 commits intoindustrial-optimization-group:masterfrom
Conversation
desdeo/api/routers - refactor
| parent_state=parent_state, | ||
| ) | ||
|
|
||
| def get_session_context_base( |
There was a problem hiding this comment.
Is this really needed since calling SessionContext(user=..., db_session=...) is already equivalent to this?
There was a problem hiding this comment.
Now, I think it is. Tests failed when I tried to use get_session_context() method at endpoint which do not contain request data. So I created a method, which does not require request parameter.
When I asked Chatbot for help, got answer like this -
"Because FastAPI dependencies must be callable, and you cannot use a union type RequestType as a dependency.
The only way to reuse the same context structure in endpoints without request data is to provide a dedicated dependency function."
But if is it wrong way of thinking, let me know.
There was a problem hiding this comment.
I see, this makes sense then. I renamed the function to be more descriptive.
There was a problem hiding this comment.
Good, thank you!
See PtrBednarik#4 (some changes I made while reviewing that should be incorporated). Please merge these into your branch when updating the PR. Check my comments for more details what needs to change. I might not have added a comment everywhere were a repeated issues has risen (e.g., with the missing docstrings), so please generalize when needed.
Some notes:
- Refrain from reducing the documentation level of the code. Now, in some places docstrings were completely removed, and in some places important comments were removed. I do not understand why?
- I suggest you setup pre-commit hooks, see https://desdeo.readthedocs.io/en/latest/tutorials/contributing/#pre-commit-hooks (feedback is welcomed on the section!)
Get session context
540991d
into
industrial-optimization-group:master
#405