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

Add config option to skip Touchup step, for debugging purposes #2361

Merged
merged 9 commits into from Mar 24, 2022

Conversation

ashleysommer
Copy link
Member

This new option allows you to configure the App to skip the Touchup step at startup.
This is useful for debugging purposes, it means you can again set breakpoints into sanic methods like handle_request and have them work as expected at runtime.

This option also sets fail_not_found to False for built-in system signals, so the code can still dispatch signals and not get a NotFound exception.

Tests added for coverage.

sanic/app.py Outdated Show resolved Hide resolved
sanic/app.py Outdated Show resolved Hide resolved
ahopkins and others added 3 commits January 18, 2022 11:44
…tartup.

Add a test to ensure user-defined signals will still throw not-found.
Add comment to cli args about future change in default behaviour.
# Conflicts:
#	sanic/app.py
#	sanic/cli/app.py
#	sanic/cli/arguments.py
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #2361 (0334411) into main (f6fdc80) will increase coverage by 0.117%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##              main     #2361       +/-   ##
=============================================
+ Coverage   87.109%   87.226%   +0.117%     
=============================================
  Files           60        60               
  Lines         5027      5034        +7     
  Branches       905       907        +2     
=============================================
+ Hits          4379      4391       +12     
+ Misses         475       472        -3     
+ Partials       173       171        -2     
Impacted Files Coverage Δ
sanic/app.py 89.230% <100.000%> (+0.083%) ⬆️
sanic/config.py 99.295% <100.000%> (+0.004%) ⬆️
sanic/signals.py 92.537% <100.000%> (+3.900%) ⬆️

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 f6fdc80...0334411. Read the comment docs.

@ashleysommer
Copy link
Member Author

ashleysommer commented Jan 19, 2022

I've implemented the changes as suggested above.
I also added a message to say that in v22.3 --debug switch will skip runtime optimizations, but when I merged main it pulled in the remove-warnings changeset, which removed those v22.3 messages, so I removed my new messages too.

Still, I'd like to see --debug apply app.config.SKIP_TOUCHUP=True in Sanic v22.3

@ahopkins
Copy link
Member

Still, I'd like to see --debug apply app.config.SKIP_TOUCHUP=True in Sanic v22.3

That seems reasonable. I would also apply that in _startup:

if app.state.is_debug:
    app.config.SKIP_TOUCHUP = True

One more question though: do you think it would be better if the config were not in the negative?

app.config.TOUCHUP = False

@ahopkins
Copy link
Member

@ashleysommer How is this looking? Can we get this merged soon?

@ashleysommer
Copy link
Member Author

Oh, sorry. I thought I'd finished with this. I'll check if I made those changes in my branch.

@ashleysommer
Copy link
Member Author

One more question though: do you think it would be better if the config were not in the negative?
app.config.TOUCHUP = False

Yeah, if we have TOUCHUP as a permanent configuration option that will always be present in app.config, it can default to True, and I'm happy to swap the logic around.

Now set TOUCHUP config to False when running in debug mode
@ashleysommer
Copy link
Member Author

Good to go now

ahopkins
ahopkins previously approved these changes Mar 24, 2022
@ahopkins
Copy link
Member

Nice 😎

Looks like there was one place where the config swap was missed. Just pushed the fix for you.

@ahopkins
Copy link
Member

I have not tested this yet, but this might also solve some of the problems with test coverage 🤔. Maybe this is something I will check next week after the release.

@ashleysommer
Copy link
Member Author

Looks like there was one place where the config swap was missed. Just pushed the fix for you.

Doh! I missed the second test. Thanks.

@ahopkins ahopkins merged commit 0309874 into sanic-org:main Mar 24, 2022
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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants