Skip to content

[wip] Added undo operation for xeus-cpp #293

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kr-2003
Copy link
Contributor

@kr-2003 kr-2003 commented May 1, 2025

Description

Undo last N operations.

Partially addresses #263

Screenshot 2025-05-01 at 12 53 29 PM

@kr-2003 kr-2003 marked this pull request as draft May 1, 2025 07:20
@kr-2003 kr-2003 changed the title Added undo operation for xeus-cpp [wip] Added undo operation for xeus-cpp May 1, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@anutosh491
Copy link
Collaborator

Maybe you can add a couple tests ?

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 3, 2025

Maybe you can add a couple tests ?

Sure.

@kr-2003 kr-2003 force-pushed the undo branch 3 times, most recently from 889fa08 to 80f6216 Compare May 23, 2025 10:07
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 17. Check the log or trigger a new build to see more.

@codecov-commenter
Copy link

codecov-commenter commented May 23, 2025

Codecov Report

Attention: Patch coverage is 48.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 80.70%. Comparing base (97daaeb) to head (1976291).

Files with missing lines Patch % Lines
src/xinterpreter.cpp 48.00% 13 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
- Coverage   81.78%   80.70%   -1.09%     
==========================================
  Files          20       20              
  Lines         950      969      +19     
  Branches       87       90       +3     
==========================================
+ Hits          777      782       +5     
- Misses        173      187      +14     
Files with missing lines Coverage Δ
src/xinterpreter.cpp 84.07% <48.00%> (-6.04%) ⬇️
Files with missing lines Coverage Δ
src/xinterpreter.cpp 84.07% <48.00%> (-6.04%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

@anutosh491
Copy link
Collaborator

Hey @kr-2003,

I see lot of formatting changes and stuff here. Let's keep implementation prs and refactoring/formatting prs separately.

I know sometimes the ci would suggest some edits, but that's just overhead. We shouldn't necessarily respect it everytime as it just pollutes commit history.

Could you please clean this up to just have the core changes required here to enable undo ?

@mcbarton
Copy link
Collaborator

Fascinating that the Emscripten tests pass for the MacOS build, but not the Linux Emscripten build here https://github.com/compiler-research/xeus-cpp/actions/runs/15210235224

@anutosh491
Copy link
Collaborator

anutosh491 commented May 23, 2025

Fascinating that the Emscripten tests pass for the MacOS build, but not the Linux Emscripten build here

Fascinatingly weird is what I would say.

Cause undo working with xeus-cpp-lite would take a good time. I don't even see this happening in the near future.

@kr-2003 could you please look into what's the failure with the emscripten build ? (I don't think there should be any changes with respect to the emscripten build)

That being said as cppitnerop 1.7.0 is supporting undo, having undo natively would be really good.

EDIT: Also I'll have a go at the code soon but I am guessing we're not registering undo as a magic right now and just using it as clang-repl does !

@mcbarton mcbarton requested a review from vgvassilev May 23, 2025 13:56
@mcbarton
Copy link
Collaborator

mcbarton commented May 23, 2025

@kr-2003 can you add to the example notebook we use for native builds, showing the undo feature in action?

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 23, 2025

@kr-2003 can you add to the example notebook we use for native builds, showing the undo feature in action?

Sure, I’ll add it in the meantime

@vgvassilev
Copy link
Contributor

We might need to rethink the entire meaning of undo in the context of jupyter notebooks. We probably want to change the granularity of the undo operations to be on cell level. In that case, however, we will need some level of support to "grey-out" the undone cells...

@anutosh491
Copy link
Collaborator

We might need to rethink the entire meaning of undo in the context of jupyter notebooks.

Hmm, I thought we would just stick to what clang-repl/cppinterop was doing.

"%undo" looks like an interesting trick though haha ( so I thought we could introduce it as a magic ... %%undo I guess ?)

@vgvassilev
Copy link
Contributor

We might need to rethink the entire meaning of undo in the context of jupyter notebooks.

Hmm, I thought we would just stick to what clang-repl/cppinterop was doing.

"%undo" looks like an interesting trick though haha ( so I thought we could introduce it as a magic ... %%undo I guess ?)

The question is what would be the usability aspect of this. The main usecase is that the repl has linear input sequence and this is the only way to remove already executed code and reuse the names. The granularity for jupyter is a cell which is multiple statements. It is more similar to the .L feature of Cling where we insert a watermark to undo file inclusion. In that case we can undo a cell.

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 24, 2025

We might need to rethink the entire meaning of undo in the context of jupyter notebooks. We probably want to change the granularity of the undo operations to be on cell level. In that case, however, we will need some level of support to "grey-out" the undone cells...

Just to clarify — when you mention rethinking the meaning of undo and changing the granularity to the cell level, do you mean that using %undo should erase all the statements from the previous cell?

If so, it seems like %undo already handles that behavior, as shown here:
undo

In that case, the only remaining enhancement might be adding support for visually greying-out the undone cells.

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.

5 participants