-
Notifications
You must be signed in to change notification settings - Fork 29k
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
Ensure nb cell source uses LF instead of CRLF #133762
Conversation
@@ -91,6 +91,7 @@ export class NotebookSerializer implements vscode.NotebookSerializer { | |||
const indentAmount = data.metadata && 'indentAmount' in data.metadata && typeof data.metadata.indentAmount === 'string' ? | |||
data.metadata.indentAmount : | |||
' '; | |||
return JSON.stringify(notebookContent, undefined, indentAmount); | |||
// ipynb always ends with a trailing new line (we add this so that SCMs do not show unnecesary changes, resulting from a missing trailing new line). | |||
return JSON.stringify(sortObjectPropertiesRecursively(notebookContent), undefined, indentAmount) + '\n'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ipynb have a trailing \n
* Jupyter notbeooks/labs sorts the JSON keys in alphabetical order. | ||
* https:/microsoft/vscode-python/issues/13155 | ||
*/ | ||
export function sortObjectPropertiesRecursively(obj: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was missed when moving the serializer into vscode, adding this back.
I think at one point we relialized this was missing, can't remember. but added comments with the link to the original issue so we have some history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is an issue on the vscode side for it but I thought we figured out that it wasn't actually being done in the other serializer at the time we moved it
function createCodeCellFromNotebookCell(cell: NotebookCellData): nbformat.ICodeCell { | ||
const cellMetadata = cell.metadata?.custom as CellMetadata | undefined; | ||
const codeCell: any = { | ||
cell_type: 'code', | ||
execution_count: cell.executionSummary?.executionOrder ?? null, | ||
source: splitMultilineString(cell.value), | ||
source: splitMultilineString(cell.value.replace(/\r\n/g, '\n')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use \n
instead of \r\n
@@ -81,7 +81,7 @@ export class NotebookSerializer implements vscode.NotebookSerializer { | |||
const notebookContent: Partial<nbformat.INotebookContent> = data.metadata?.custom || {}; | |||
notebookContent.cells = notebookContent.cells || []; | |||
notebookContent.nbformat = notebookContent.nbformat || 4; | |||
notebookContent.nbformat_minor = notebookContent.nbformat_minor || 2; | |||
notebookContent.nbformat_minor = notebookContent.nbformat_minor ?? 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a Bug, if the value was 0
we were replacing with 2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the above? Maybe that can't happen
/** | ||
* Cell id for notebooks created with the new 4.5 version of nbformat. | ||
*/ | ||
id?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brought in a PR that was in jupyter extnesion to support forwards compatibility with the new notebook format 4.5
microsoft/vscode-jupyter#5970
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new property isn't required in older versions, and only required in the latest 4.5 version (since we support older versions we're not going to upgrade to 4.5, but ensure users can create notebooks in format 4.2 and open notebooks created using format 4.5)
Note: Assume user creates a notebook in vscode (or older version of jupyter) & then opens this in latest version of Jupyter, the notebook format is upgrade to version 4.5 & jupyter notebook/lab will add the new id attribute.
Unfortunately I don't have windows, will need to test this out on windows, give me some time to get this tested.
This PR fixes #125132