Fixes bugs #11777 and #11726, 'outputPrepend' needed to be cleared on each cell execute.#11871
Fixes bugs #11777 and #11726, 'outputPrepend' needed to be cleared on each cell execute.#11871rchiodo merged 7 commits intomicrosoft:masterfrom BarryNolte:Truncation-Message-Fix
Conversation
Changed 'outputPrepend' to be a per 'execution' tag instead of a per 'session' tag. Once it got saved in the ipynb file, it never went away, thus always showing the truncation message. All fixed an egregious spelling error.
| const data: nbformat.ICodeCell = cell.data as nbformat.ICodeCell; | ||
| let originalTextLenght = 0; | ||
| let trimmedTextLenght = 0; | ||
| let originalTextLength = 0; |
There was a problem hiding this comment.
Also not sure how this addresses https://github.com/microsoft/vscode-python/issues/11777
I think we need some tests to confirm the fix.
Again will wait for feedback from the rest of the team.
There was a problem hiding this comment.
The spelling error fix is a side effect of fixing the actual bug, as in, leave the area nicer than you found it. Clearing the tag, fixes the bug. The truncating always worked, it was when the warning showed up that didn't.
| await this.clearResult(cell.id); | ||
|
|
||
| // Clear 'per run' data passed to WebView before execution | ||
| cell.data.metadata.tags = []; |
There was a problem hiding this comment.
Thanks for submitting this PR, however I fail to see how this will solve the issue.
Doesn't this just blow away all of the tags in the metadata, meaning we're throwing away client data stored in tags.
Will wait for feedback from the rest of the team.
There was a problem hiding this comment.
At the moment, this is the only tag that ever gets passed over to the client. Until there is another one added, it's just a single complicated flag.
There was a problem hiding this comment.
Yeah this should only be clearing the flag we add. Other metadata tags may have already been there and we don't want to clear them.
In reply to: 426679031 [](ancestors = 426679031)
There was a problem hiding this comment.
Additionally we should not blindly push the outputPrepend tag in jupyterNotebook.ts
In reply to: 426727089 [](ancestors = 426727089,426679031)
There was a problem hiding this comment.
Hi Barry, should have been more clear there.
cell.data.metadata.tags can be written to by external tools (like this one https://ipypublish.readthedocs.io/en/latest/metadata_tags.html)
We added the outputprepend tag, but we don't want to remove other tags that might already be there. So something like
cell.data.metadata.tags = cell.metadata.tags.filter(t => t !== 'outputPrepend')
would be better.
The real bug is that we're just blindly pushing the outputPrepend tag instead of doing it just once.
There was a problem hiding this comment.
Got it. My repro case is too simplistic, as in no external tools involved. That starts to explain a lot. This is a rabbit hole :). I'll submit yet another PR.
DonJayamanne
requested review from
DavidKutu,
DonJayamanne,
IanMatthewHuff,
greazer and
rchiodo
Changed 'outputPrepend' to be a per 'execution' tag instead of a per 'session' tag. Once it got saved in the ipynb file, it never went away, thus always showing the truncation message. All fixed an egregious spelling error.
…e/vscode-python into Truncation-Message-Fix # Conflicts: # src/client/datascience/interactive-ipynb/nativeEditor.ts
…e/vscode-python into Truncation-Message-Fix # Conflicts: # src/client/datascience/interactive-ipynb/nativeEditor.ts
…e/vscode-python into Truncation-Message-Fix
…e/vscode-python into Truncation-Message-Fix
…e/vscode-python into Truncation-Message-Fix
|
Kudos, SonarCloud Quality Gate passed!
|
4 participants
Changed 'outputPrepend' to be a per 'execution' tag instead of a per 'session' tag. Once it got saved in the ipynb file, it never went away, thus always showing the truncation message. All fixed an egregious spelling error.
Has unit tests & system/integration testspackage-lock.jsonhas been regenerated by runningnpm install(if dependencies have changed).