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 changing cell type from python #5950

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Conversation

seeM
Copy link
Contributor

@seeM seeM commented Jan 10, 2025

Release Notes

New Features

  • N/A

Bug Fixes

QA Notes

See #4160 for the repro.

@seeM seeM requested a review from austin3dickey January 10, 2025 15:47
Copy link

github-actions bot commented Jan 10, 2025

E2E Tests 🚀  ?
This PR will run tests tagged with: @critical


positron_did_close_diagnostics(server, params)

# Wait for the diagnostics to be published
Copy link
Contributor Author

@seeM seeM Jan 10, 2025

Choose a reason for hiding this comment

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

I was hesitant to add such a slow test (~1 second). Might be worth making the debounce behavior configurable and setting it to zero during tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also worth considering whether we actually want diagnostics to be debounced?

That was inherited from upstream (pappasam/jedi-language-server#241) but I'm not sure which is a better UX.

Copy link
Contributor

Choose a reason for hiding this comment

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

making the debounce behavior configurable and setting it to zero during tests

Yeah, maybe this is a good idea, or at least something like 100ms so we know there is some debouncing?

whether we actually want diagnostics to be debounced

I like a bit of debouncing. Maybe 1s is too much though. Probably depends widely on the user but maybe 0.5s is okay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I've sped up the tests.

0.5 and 1s feel close enough that I think we can leave it as is for now and see if we get any user feedback / strong opinions.

@seeM seeM merged commit af026da into main Jan 15, 2025
27 checks passed
@seeM seeM deleted the 4160-fix-changing-cell-type-to-raw branch January 15, 2025 05:35
@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2025
@posit-dev posit-dev unlocked this conversation Jan 15, 2025
@posit-dev posit-dev locked and limited conversation to collaborators Jan 15, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants