Skip to content

Add language server support for native interactive window#16560

Merged
joyceerhl merged 27 commits into
mainfrom
dev/joyceerhl/interactive-intellisense
Jun 29, 2021
Merged

Add language server support for native interactive window#16560
joyceerhl merged 27 commits into
mainfrom
dev/joyceerhl/interactive-intellisense

Conversation

@joyceerhl
Copy link
Copy Markdown

@joyceerhl joyceerhl commented Jun 24, 2021

No description provided.

@rebornix
Copy link
Copy Markdown

rebornix commented Jun 26, 2021

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
Copy Markdown
Author

joyceerhl commented Jun 28, 2021

@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
rchiodo
rchiodo approved these changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown

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

karrtikr
karrtikr approved these changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown

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

Comment thread src/client/jupyter/languageserver/interactiveConcatTextDocument.ts
return position;
}

lineAt(posOrNumber: Position | number): TextLine {
Copy link
Copy Markdown

@karrtikr karrtikr Jun 28, 2021

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
Copy Markdown
Author

@joyceerhl joyceerhl Jun 28, 2021 •
edited
Loading

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.

paulacamargo25
paulacamargo25 approved these changes Jun 28, 2021
View reviewed changes
Copy link
Copy Markdown

@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

Reviewers

@rchiodo rchiodo rchiodo approved these changes

+2 more reviewers

@karrtikr karrtikr karrtikr approved these changes

@paulacamargo25 paulacamargo25 paulacamargo25 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@karrtikr karrtikr

Labels

no-changelog No news entry required

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joyceerhl @rebornix @karrtikr @paulacamargo25 @rchiodo