-
Notifications
You must be signed in to change notification settings - Fork 36
[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
base: main
Are you sure you want to change the base?
Conversation
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.
clang-tidy made some suggestions
Maybe you can add a couple tests ? |
Sure. |
889fa08
to
80f6216
Compare
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.
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
🚀 New features to boost your workflow:
|
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.
clang-tidy made some suggestions
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 ? |
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 |
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 ! |
@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 |
We might need to rethink the entire meaning of |
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. |
Description
Undo last N operations.
Partially addresses #263