Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix?: recursion error on Sanic subclass initialisation #2072

Merged
merged 4 commits into from
Mar 21, 2021

Conversation

artcg
Copy link
Contributor

@artcg artcg commented Mar 20, 2021

Fixes #2073

Needs someone to take a look at it, not super familiar with __new__,

but it seems to fix the error and work as expected

@artcg artcg requested a review from a team as a code owner March 20, 2021 16:56
@codecov
Copy link

codecov bot commented Mar 20, 2021

Codecov Report

Merging #2072 (c9c888d) into master (15a8b5c) will decrease coverage by 0.016%.
The diff coverage is 100.000%.

Impacted file tree graph

@@              Coverage Diff              @@
##            master     #2072       +/-   ##
=============================================
- Coverage   92.200%   92.184%   -0.016%     
=============================================
  Files           38        38               
  Lines         3487      3480        -7     
  Branches       584       582        -2     
=============================================
- Hits          3215      3208        -7     
  Misses         184       184               
  Partials        88        88               
Impacted Files Coverage Δ
sanic/base.py 100.000% <100.000%> (ø)
sanic/blueprints.py 100.000% <100.000%> (ø)

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 15a8b5c...c9c888d. Read the comment docs.

@artcg
Copy link
Contributor Author

artcg commented Mar 20, 2021

Would also add a test case for this if subclassing Sanic is something that is officially supported...

@ahopkins ahopkins self-assigned this Mar 21, 2021
@ahopkins
Copy link
Member

@artcg Thanks. Can you add a test? You can drop it at the end of test_app.py. It is something that I know is done, so we should have that in the test suite.

Copy link
Member

@ahopkins ahopkins left a comment

Choose a reason for hiding this comment

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

This works and makes sense. Nice catch.

But, I want to play with this a little more because I think it might not yet cover all the edge cases.

Let's add the test, and I will swing back on this tomorrow with some more thoughts.

sanic/base.py Outdated
bases = [
b for base in type(self).__bases__ for b in base.__bases__
]
bases = BaseSanic.__bases__

for base in bases:
Copy link
Member

Choose a reason for hiding this comment

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

Since we no longer need the comprehension, no need to have bases as a variable.

for base in BaseSanic.__bases__:

@artcg
Copy link
Contributor Author

artcg commented Mar 21, 2021

Is it an optimisation not just to define BaseSanic.__init__ directly?

@ahopkins
Copy link
Member

I am pushing a change directly to this branch to remove the metaclass.

@ahopkins ahopkins merged commit 6763e2b into sanic-org:master Mar 21, 2021
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.

RecursionError on Sanic subclass initialisation
2 participants