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

Remove app instance from Config for error handler setting #2320

Merged
merged 16 commits into from
Dec 18, 2021

Conversation

ahopkins
Copy link
Member

@ahopkins ahopkins commented Nov 21, 2021

This stems from a conversation with @prryplatypus and @ashleysommer re: the change to get the ErrorHandler functioning as expected in v21.9. This unfortunately introduced app as a property of the Config object.

This PR removes that, reverting back to the previous methodology, but still retaining the as expected ability to apply changes to the Config post-instantiation and carry through to the route handler that was missing in earlier v21.

TODO

  • How to handle post-init change to Config where ErrorHandler is not default
  • Value checking when setting fallback

@codeclimate
Copy link

codeclimate bot commented Nov 21, 2021

Code Climate has analyzed commit 918b06f and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 86.6% (86% is the threshold).

This pull request will bring the total coverage in the repository to 86.8% (0.0% change).

View more on Code Climate.

ashleysommer
ashleysommer previously approved these changes Nov 22, 2021
Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Thanks for making this change

sanic/handlers.py Show resolved Hide resolved
sanic/handlers.py Outdated Show resolved Hide resolved
sanic/handlers.py Outdated Show resolved Hide resolved
@ahopkins ahopkins marked this pull request as ready for review December 14, 2021 10:17
@ahopkins ahopkins requested a review from a team as a code owner December 14, 2021 10:17
@ahopkins ahopkins self-assigned this Dec 14, 2021
@ahopkins ahopkins added the needs review the PR appears ready but requires a review label Dec 14, 2021
@ahopkins ahopkins marked this pull request as draft December 14, 2021 10:25
@ahopkins ahopkins marked this pull request as ready for review December 14, 2021 12:24
Copy link
Member

@prryplatypus prryplatypus left a comment

Choose a reason for hiding this comment

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

LGTM except the DescriptorMeta in the Config class, because I don't understand what it does 😅

@ahopkins
Copy link
Member Author

It stores an internal reference to descriptors so that when setting a value we determine if we need to execute the setter or fallback to the dict update.

@ahopkins ahopkins merged commit abe062b into main Dec 18, 2021
@ahopkins ahopkins deleted the decouple-config-errorhandler-app branch December 18, 2021 16:58
ChihweiLHBird pushed a commit to ChihweiLHBird/sanic that referenced this pull request Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review the PR appears ready but requires a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants