Skip to content
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

Add a copy capability to jupyter notebooks #3071

Closed
wants to merge 4 commits into from

Conversation

agrippa
Copy link

@agrippa agrippa commented Nov 22, 2017

This is a small change to add the ability to copy files in the Notebook UI, in addition to moving them. Because of my infamiliarity with the notebook code base, I tried to minimize the amount of changes necessary to achieve this. Mostly, this appropriates the same code paths as Move but adds a keep_old flag that indicates whether the source file should be cleared out (move) or not (copy).

This is my first pull request on the repo, so apologies if I skipped/mangled a whole bunch of steps in creating this pull request (as I imagine I may have).

@takluyver
Copy link
Member

Thanks! It will probably be a few days before I can look at this properly, but someone else might be able to have a look sooner.

It looks like the CI tests have failed - can you have a look and see if that's related to the change you made?

There is already a method ContentsManager.copy() in the API - maybe it's possible to use that?

@@ -73,7 +73,7 @@ define(function(){
// tree
jglobal('SessionList','tree/js/sessionlist');

Jupyter.version = "5.2.1";
Jupyter.version = "5.3.0.dev0";

Choose a reason for hiding this comment

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

@agrippa I think version changes automatically when start developing the code base. And should be changed when we are making a PR, if I'm not mistaken.
I'm currently new to the codebase so I might be wrong in this. 😄

@takluyver
Copy link
Member

ContentsManager already has a copy() method - can we try using that rather than adding a parameter to rename()? Or is there some reason that won't work?

@agrippa
Copy link
Author

agrippa commented Jan 9, 2018

@takluyver Thanks for the feedback! I looked at just using copy() and did a bit of refactoring, is that along the lines of what you were thinking?

@takluyver
Copy link
Member

That's a start, but I don't think we need the keep_old parameter anywhere - the contents manager class already has a copy() method separate from rename(). Maybe we need to expose it to the notebook UI.

@Zsailer
Copy link
Member

Zsailer commented May 15, 2020

Thanks, @agrippa, for this work!

This has been inactive for awhile, so I'm going to close this PR for now.

I think it's a good feature to add at some point, but it looks like developer time is limited here. If anyone has time to devote to it, feel free reopen or resubmit a new PR.

Thanks!

@Zsailer Zsailer closed this May 15, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants