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 a language switcher that auto detects magics #8754

Closed
wants to merge 1 commit into from

Conversation

rchiodo
Copy link
Contributor

@rchiodo rchiodo commented Jan 20, 2022

Fixes #4588

Added a new class 'LanguageSwitcher' that watches for cell magics that switch a cell to a different type.

Basically if you type something like %%html the cell will auto switch to html.

@rchiodo rchiodo requested a review from a team as a code owner January 20, 2022 19:15
}
}

private hasPythonController(cellDocument: TextDocument): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should go into the core VS Code extension for Jupyter notebooks.
This change will work well for new notebooks and adding cells.
But if you save this notebook and re-open it, the language will not be preserved.
Also if you open an existing notebook, the languages will still be python, when they should have been changed based on the magics.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was just looking in the core code recently. It's seems like there is a deeper issue here where language selection isn't preserved in the file at all.

Isn't that a bigger issue though? If I choose some wacky language I'd expect VS Code to save and honor that. For example if I have '%%html' and choose SQL manually in the language selector I'd expect to get SQL when I reopen the file. Moving this into deserialize cell preferred language doesn't fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this into deserialize cell preferred language doesn't fix that.

It will, if we save the language cell in a metadata of the cell.
Thought I'm not sure why anyone would change a cell from html to sql when one has %%html magic. possible yes, but why? Again we can address this but saving this information in the cell metadata. I don't think VS Code should try to save this information elsewhere, else it could be argued that once I share this file with someone else the same cell is no longer sql but treated as html/python, i.e. the saved info is still not preserved.

Copy link
Member

Choose a reason for hiding this comment

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

I think that I was thinking of my quick investigation into this issue some:
microsoft/vscode#140673

This customer is using magics so making this change in core would be good for them. But when investigating on my own without the magics in the cell it was pretty surprising to me that it wouldn't honor the manual language selections that I had made on file save and reopen. I guess I don't see the point of the manual language selector if it's not honored in that situation. Would we expect someone to change it to a different language just temporarily?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize the core wasn't saving the language in the metadata. Yeah this should probably be done in the core jupyter extension. And it should do this on open.

Although if we did this on open, it wouldn't have to be in the core language extension. It would just happen on every open.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would just happen on every open.

Then we'd be marking documents as dirty when ever a document is opened.
Hence my argument for setting it in core, also in core today we alraedy set the cell langauge as python (hence setting language as html shoudl also be in core - i.e. serializer)

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets discuss in standup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize changing cell language makes it dirty. Seems weird if it doesn't actually write anything to metadata.

Yeah serializer has to do it if we want to save it and not have the file be dirty.

Copy link
Contributor

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

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

Best for core Jupyter extension in VS Code to load cells in the right language.

@codecov-commenter
Copy link

Codecov Report

Merging #8754 (710fc79) into main (d712c97) will decrease coverage by 0%.
The diff coverage is 97%.

@@          Coverage Diff          @@
##            main   #8754   +/-   ##
=====================================
- Coverage     71%     71%   -1%     
=====================================
  Files        381     382    +1     
  Lines      24535   24576   +41     
  Branches    3777    3780    +3     
=====================================
+ Hits       17654   17680   +26     
- Misses      5361    5373   +12     
- Partials    1520    1523    +3     
Impacted Files Coverage Δ
src/client/common/types.ts 100% <ø> (ø)
...rc/client/datascience/notebook/languageSwitcher.ts 96% <96%> (ø)
src/client/common/configSettings.ts 86% <100%> (+<1%) ⬆️
src/client/datascience/serviceRegistry.ts 98% <100%> (+<1%) ⬆️
...ience/variablesView/variableViewMessageListener.ts 77% <0%> (-23%) ⬇️
...ent/common/application/webviewViews/webviewView.ts 67% <0%> (-11%) ⬇️
src/client/datascience/prereleaseChecker.ts 64% <0%> (-9%) ⬇️
src/client/common/application/applicationShell.ts 22% <0%> (-5%) ⬇️
.../client/datascience/debugLocationTrackerFactory.ts 92% <0%> (-4%) ⬇️
src/client/common/application/webviews/webview.ts 73% <0%> (-2%) ⬇️
... and 5 more

@rchiodo rchiodo closed this Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support language features for other languages when using cell magics
4 participants