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 language server support for native interactive window #16560

Merged
merged 27 commits into from
Jun 29, 2021

Conversation

joyceerhl
Copy link

No description provided.

@rebornix
Copy link
Member

Right now when we close a notebook with diagnostics, the diagnostics are never cleared due to the flow below

open interactive_1 notebook
open interactive_1#ch00000 markdown
open interactive_1#ch00001 python
open dummy_file_abc.py


close interactive_1#ch00000 markdown, ignore
close interactive_1  remove wrapper for dummy_file_abc.py
close interactive_1#ch00001
    create wrapper, dummy_file_def.py
    next(close, dummy_file_def.py)

@joyceerhl joyceerhl requested a review from rchiodo June 28, 2021 21:04
@joyceerhl
Copy link
Author

@rchiodo, @rebornix and I would really appreciate your review on the initial work we have here. Per this morning's discussion there are still some other issues with clearing diagnostics that we will resolve in a separate PR.

@rebornix rebornix marked this pull request as ready for review June 28, 2021 21:12
@joyceerhl joyceerhl added the no-changelog No news entry required label Jun 28, 2021
Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

Looks good. I think I'd want a unit test to verify the concat stuff in the interactive concat document.

Copy link

@karrtikr karrtikr left a comment

Choose a reason for hiding this comment

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

I see no testing, do we plan to do it separately, or is it not needed? Otherwise, LGTM.

return position;
}

lineAt(posOrNumber: Position | number): TextLine {

Choose a reason for hiding this comment

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

Curious, why are scopes not explicitly declared for these methods? Is it because it means they are public methods by default?

Copy link
Author

@joyceerhl joyceerhl Jun 28, 2021

Choose a reason for hiding this comment

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

Will add a unit test this week, and there are still some other bugs with diagnostics not being cleared on document close that we will fix up in a separate PR. And yes all class members are public by default, and these methods are also supposed to be public.

Copy link

@paulacamargo25 paulacamargo25 left a comment

Choose a reason for hiding this comment

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

LGTM!

@joyceerhl joyceerhl merged commit b97942d into main Jun 29, 2021
@joyceerhl joyceerhl deleted the dev/joyceerhl/interactive-intellisense branch June 29, 2021 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants