Skip to content

Simplify code syntax in several places#1753

Merged
tiangolo merged 2 commits intofastapi:masterfrom
uriyyo:comperhensions_base_classes
Aug 3, 2020
Merged

Simplify code syntax in several places#1753
tiangolo merged 2 commits intofastapi:masterfrom
uriyyo:comperhensions_base_classes

Conversation

@uriyyo
Copy link
Contributor

@uriyyo uriyyo commented Jul 21, 2020

Hi guys,
I enjoy to work with FastAPI, it's really great 👍

What was done in scope of this PR:

  • Use comprehension instead of loops.
  • Use __init__ method of a base class where it possible.
  • Replace usage of dict.setdefault method with collections.defaultdict.

@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #1753 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1753   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          235       235           
  Lines         6987      6985    -2     
=========================================
- Hits          6987      6985    -2     
Impacted Files Coverage Δ
fastapi/applications.py 100.00% <100.00%> (ø)
fastapi/dependencies/utils.py 100.00% <100.00%> (ø)
fastapi/encoders.py 100.00% <100.00%> (ø)
fastapi/openapi/constants.py 100.00% <100.00%> (ø)
fastapi/security/http.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72645df...d691cc6. Read the comment docs.

Copy link

@acutaia acutaia left a comment

Choose a reason for hiding this comment

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

Add another comprehension

Copy link

@acutaia acutaia left a comment

Choose a reason for hiding this comment

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

Add another comprhension

@jahhhsueihh
Copy link

jahhhsueihh commented Jul 21, 2020 via email

@tiangolo tiangolo force-pushed the comperhensions_base_classes branch from ee8242d to d691cc6 Compare August 3, 2020 13:07
@tiangolo tiangolo changed the title Use comprehensions instead of loops Simplify code syntax in several places Aug 3, 2020
@tiangolo tiangolo merged commit 55b9fae into fastapi:master Aug 3, 2020
@tiangolo
Copy link
Member

tiangolo commented Aug 3, 2020

Thanks for your effort @uriyyo ! 🚀

And thanks for the discussion everyone. ☕

Nice trick with the defaultdict! 🤓

I reverted a couple of the changes, some lists, because I find it more explicit with an explicit list than with a list comprehension when the logic starts to have some extra complexity.

For some of the classes, I had intentionally not used super() at some point after some breaking changes in Starlette, and because there wasn't that much shared code, I had decided to duplicate the small bits, given I still had to implement the custom local logic.

And for the security classes, they don't really have any internal logic, they only set attributes, so I found it more explicit to duplicate those little bits that still needed some customization for each specific class in several cases than to pass everything to super(), as it seems to hide some virtual complexity that doesn't really exist. The amount of code is quite similar, and it's still needed to override some things (like the Pydantic models). So I prefer to keep it that way.

Thanks for your contribution! 🚀 🍰

@uriyyo uriyyo deleted the comperhensions_base_classes branch August 3, 2020 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants