Skip to content

Conversation

@bokshitsky
Copy link
Contributor

@bokshitsky bokshitsky commented Oct 26, 2025

TLDR: just simplify internal logic and code a bit, doesn't change framework behaviour

split in in 5 commits:

  1. remove unused variable initialization errors: List[Any] = []

  2. simplify code: remove nested code, using early exit pattern

if errors:
  raise Exception()
  1. instead of:
current_status_code = (
    status_code if status_code else solved_result.response.status_code
)
if current_status_code is not None:
    response_args["status_code"] = current_status_code
if solved_result.response.status_code:
    response_args["status_code"] = solved_result.response.status_code

write:

if solved_result.response.status_code:
    response_args["status_code"] = solved_result.response.status_code
elif status_code is not None:
    response_args["status_code"] = status_code

if remains exactly the old behaviour, but
a) looks more clear (right now looks like status_code if status_code else solved_result.response.status_code is default value, but actually in the end default value is solved_result.response.status_code
b) it doesn't make any speed effect, but new version use less comparison and assignment operations

  1. remove non used variable response initialization

raise http_error from e

# Solve dependencies and run path operation function, auto-closing dependencies
errors: List[Any] = []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable is overwritten by errors = solved_result.errors a bit lower

@bokshitsky bokshitsky force-pushed the simplify_deps_code branch 2 times, most recently from 6007760 to dba23ba Compare October 26, 2025 18:42
is_body_form = body_field and isinstance(
body_field.field_info, (params.Form, temp_pydantic_v1_params.Form)
)
if is_body_form:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't change internal rules, just simplify code removing nesting

raise RequestValidationError(_normalize_errors(errors), body=body)

# Return response
assert response
Copy link
Contributor Author

@bokshitsky bokshitsky Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert response was used to avoid forget response assignment inside if-else-branches, not required in proposed changes

@bokshitsky bokshitsky changed the title Simplify code in fastapi/routing [wip] simplify code in fastapi/routing Oct 27, 2025
@YuriiMotov YuriiMotov marked this pull request as draft October 27, 2025 20:20
@bokshitsky bokshitsky changed the title [wip] simplify code in fastapi/routing simplify code in fastapi/routing Oct 28, 2025
@bokshitsky
Copy link
Contributor Author

bokshitsky commented Oct 28, 2025

@YuriiMotov hi!

I removed last commit (which extracted get_request_body function) since it requires more accurate refactoring and type-annotation updates (right now body can be of type bytes, but solve_dependencies accepts only body: Optional[Union[Dict[str, Any], FormData]])

So PR is ready, may you please remove Draft tag and help with review if you have time?

@YuriiMotov YuriiMotov marked this pull request as ready for review October 28, 2025 14:26
@YuriiMotov YuriiMotov changed the title simplify code in fastapi/routing ♻️ Simplify code in fastapi.routing.py Oct 28, 2025
@YuriiMotov YuriiMotov changed the title ♻️ Simplify code in fastapi.routing.py ♻️ Simplify code in fastapi/routing.py Oct 28, 2025
Copy link
Member

@YuriiMotov YuriiMotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

With these changes code is more readable. Changes don't affect final result

@bokshitsky, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants