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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect reraise setting #571

Merged
merged 7 commits into from
Aug 24, 2021
Merged

Respect reraise setting #571

merged 7 commits into from
Aug 24, 2021

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Aug 13, 2021

We have this nice setting for having extension load failure raise errors instead of silently failing. We should respect it 馃槃

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

馃槃 - good catch Vidar. Just had the one comment. I was not aware of this option.

jupyter_server/extension/manager.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #571 (1aa5512) into master (90f619c) will increase coverage by 0.40%.
The diff coverage is 98.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   77.12%   77.52%   +0.40%     
==========================================
  Files         109      109              
  Lines       10115    10171      +56     
  Branches     1089     1096       +7     
==========================================
+ Hits         7801     7885      +84     
+ Misses       1929     1897      -32     
- Partials      385      389       +4     
Impacted Files Coverage 螖
jupyter_server/tests/test_terminal.py 98.79% <66.66%> (酶)
jupyter_server/extension/manager.py 93.89% <100.00%> (+4.09%) 猬嗭笍
jupyter_server/serverapp.py 65.53% <100.00%> (酶)
jupyter_server/tests/extension/test_app.py 98.24% <100.00%> (酶)
jupyter_server/tests/extension/test_manager.py 100.00% <100.00%> (酶)
jupyter_server/tests/services/kernels/test_cull.py 100.00% <100.00%> (酶)
...ter_server/tests/services/contents/test_manager.py 96.95% <0.00%> (-0.34%) 猬囷笍
jupyter_server/terminal/terminalmanager.py 87.50% <0.00%> (+1.25%) 猬嗭笍
jupyter_server/extension/application.py 74.31% <0.00%> (+1.37%) 猬嗭笍
...pyter_server/tests/extension/mockextensions/app.py 93.93% <0.00%> (+3.03%) 猬嗭笍
... and 1 more

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 90f619c...1aa5512. Read the comment docs.

Copy link
Member

@kevin-bates kevin-bates left a comment

Choose a reason for hiding this comment

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

Thanks Vidar - the updates look good. I'd prefer to have someone more familiar with extensions take a look as well given the move of serverapp into an instance attribute (which makes good sense to me).

cc: @Zsailer, @echarles

@echarles
Copy link
Member

Great! I like having the server starting by defaullt although some extensions fail. Keeping reraise_server_extension_failures to False by default is good. Thx @vidartf for honouring a True value in this PR.

@Zsailer Zsailer added the bug label Aug 18, 2021
@vidartf vidartf marked this pull request as ready for review August 20, 2021 11:15
@blink1073 blink1073 mentioned this pull request Aug 20, 2021
@blink1073
Copy link
Collaborator

@vidartf, can you please rebase this to pick up the formatting changes from #575? You'll have to install pre-commit locally to pick up the new formatting rules.

We have this nice setting for having extension load failure raise errors instead of silently failing. We should respect it 馃槃
So that it can always check the reraise errors flag
Supposedly this will bring me great luck and good fortune!
@vidartf
Copy link
Member Author

vidartf commented Aug 23, 2021

Rebased now. I just had to get my antivirus to stop deadlocking pre-commit 馃憤

Copy link
Collaborator

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Nice, thank you! I'll wait for @Zsailer to have a look since there are API changes

@blink1073
Copy link
Collaborator

Merging to get CI running again, we can walk back the API changes before releasing if needed. Thanks!

@blink1073 blink1073 merged commit b908f55 into master Aug 24, 2021
@blink1073 blink1073 deleted the vidartf-patch-1 branch August 24, 2021 10:29
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.

None yet

6 participants