- 
                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