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

Refactor execute_request to reduce redundancy and improve consistency #1177

Merged
merged 3 commits into from Dec 3, 2023

Conversation

jjvraw
Copy link
Contributor

@jjvraw jjvraw commented Dec 3, 2023

After reviewing #1169 again, this is somewhat a small continuation of the PR. The changes in _accepts_parameters, which now checks for two parameters, it seems redundant to verify the signature of do_execute for every cell execution request.

In addition, I just moved cell_meta and cell_id amongst the other do_execute parameter assignments for consistency.

Copy link
Member

@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.

Thanks!

@blink1073
Copy link
Member

Tests failures are unrelated: pytest-dev/pytest-asyncio#701

@jjvraw
Copy link
Contributor Author

jjvraw commented Dec 3, 2023

Sure :) I was unsure regarding the INTERNALERRORs hence the re-trigger, and was going to look into it tomorrow. But I now see the opened issue.

@blink1073
Copy link
Member

Let's try pinning the test dep to see if it clears up the failures, I'm not sure about the debugpy failure. I'll push a commit.

@blink1073
Copy link
Member

Ah, it looks like I can't make that push.

@jjvraw
Copy link
Contributor Author

jjvraw commented Dec 3, 2023

Apologies @blink1073 ! However, I noticed pytest-dev/pytest-asyncio#701 has just been resolved with v0.23.1. So, I rather just quickly pushed another empty commit to trigger with the new version.

Thanks for the quick resolve @seifertm !

@blink1073 blink1073 merged commit e35e2f5 into ipython:main Dec 3, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants