Code highlight in message #226
Conversation
|
|
brichet
force-pushed
the
code_highlight_in_message
branch
from
a4c51db to
3cdca00
Compare
brichet
force-pushed
the
code_highlight_in_message
branch
from
3cdca00 to
b4ef27a
Compare
There was a problem hiding this comment.
Thank you for working on this @brichet. I tested this PR locally and it works well. It also improves user experience as missing code highlighting in the snippets attached to the chat messages was noticeable.
Please find small suggestion on making typing of language property more explicit: #226 (comment).
| @@ -100,19 +100,22 @@ export function SendButton( | |||
|
|
|||
| function sendWithSelection() { | |||
| let source = ''; | |||
|
|
|||
| let language = ''; | |||
There was a problem hiding this comment.
If we to follow suggestion in #226 (comment), || '' part is not needed, this should be declared as
let language: string | undefined;
| } | ||
| let content = model.value; | ||
| if (source) { | ||
| content += ` | ||
|
|
||
| \`\`\` | ||
| \`\`\`${language} |
There was a problem hiding this comment.
Check for if language is undefined and replacement with empty string could be for example done here via something like language ?? ''
| @@ -148,6 +150,11 @@ export class ActiveCellManager implements IActiveCellManager { | |||
| getContent(withError: true): CellWithErrorContent | null; | |||
| getContent(withError = false): CellContent | CellWithErrorContent | null { | |||
| const sharedModel = this._notebookTracker.activeCell?.model.sharedModel; | |||
| const language = | |||
There was a problem hiding this comment.
For maintainability, clarity, and to potentially prevent misinterpretation from contributors and bugs would it make sense to use undefined to explicitly represent "not set" (then '' could explicitly represent an empty string, for example set by a user)?
| source = activeCellManager.getContent(false)!.source; | ||
| const content = activeCellManager.getContent(false); | ||
| source = content!.source; | ||
| language = content?.language || ''; |
There was a problem hiding this comment.
If we to follow suggestion in #226 (comment), || '' part is not needed
| /** | ||
| * The language of the selection. | ||
| */ | ||
| language: string; |
There was a problem hiding this comment.
If we to follow suggestion in #226 (comment), language could / should be declared as optional.
| @@ -207,13 +225,15 @@ function getTextSelection( | |||
| [start, end] = [end, start]; | |||
| } | |||
|
|
|||
| const language = await languages.getLanguage(editor?.model.mimeType); | |||
There was a problem hiding this comment.
Note getLanguage return type is Promise<IEditorLanguage | null> so it also returns "not set" in an explicit manner but using null.
| return { | ||
| ...selectionObj, | ||
| start, | ||
| end, | ||
| text, | ||
| numLines: text.split('\n').length, | ||
| widgetId: widget.id, | ||
| language: language?.name || '', |
There was a problem hiding this comment.
If we to follow suggestion in #226 (comment), if language is null it does not need to be returned at all.
This PR add code highlighting when it is attached from a cell or a selection.
The language is added to the markdown code block when it is available.
Code attached from a cell
Get the language from the
defaultKernelLanguageof the Notebook metadata. If not defined, there is no syntax highlighting.Code attached from a selection
Retrieve the
DocumentWidgetmimetype from its editor model, and convert it to a language using theEditorLanguageRegistry.If the mimetype cannot be converted to a language,there is no syntax highlighting.
Context
Fixes #222
Related to jupyterlab/jupyter-ai#1356