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

Replace copy_tree from distutils by copytree from shutil because dist… #1448

Merged
merged 5 commits into from
May 25, 2024

Conversation

samwaseda
Copy link
Member

…utils is going to be removed from python 3.12

I stumbled upon [this article](https://stackoverflow.com/questions/10047521/how-to-copy-directory-with-all-file-from-c-xxx-yyy-to-c-zzz-in-python while trying to fix the archiving problem so I did the change.

…utils is going to be removed from python 3.12
@samwaseda samwaseda requested review from pmrv and liamhuber May 24, 2024 17:26
@liamhuber
Copy link
Member

Gah, mobile trouble getting my message attached to the review, sorry. Here it is:

The PR itself looks good to me, both in terms of motivation and execution. My only concern is that the archive tests are currently kind of spotty, yeah? Since this is such a straightforward PR, integrating it on top of any other changes to the archiving should be super straightforward, so maybe we want to hold off until those issues are closed, then merge in main and rerun newly improved tests? Just to make sure there's no side effects in the switch that we're oblivious too.

I'm only keeping half an eye on the archive/import-export conversation though, so I leave it your judgement whether this concern is overblown.

@samwaseda samwaseda requested a review from liamhuber May 25, 2024 07:53
@samwaseda
Copy link
Member Author

@liamhuber I’m asking for a review again because I decided to replace all occurrences of distutils. The one that I replaced, strtobool does not seem to have a proper replacement. On the other hand it’s an independent function of only a few lines, so I copied and pasted it from the source code, which I think is vertretbar.

Coming back to your concern, now I don’t think it’s related to files, but purely database entries. Besides the two functions copy_tree and copytree are nearly identical, so I think it’s ok.

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

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

Please check under what license distutils is distributed and whether its compatible with our MIT, otherwise looks good to me.

@liamhuber
Copy link
Member

Please check under what license distutils is distributed and whether its compatible with our MIT, otherwise looks good to me.

BSD, but good point, before copy-pasting you should check that you can/what citations are needed.

The target library of shutils is just a standard library package, right? So almost certainly no worries there.

@liamhuber
Copy link
Member

Dammit, I need to figure out how to approve with a message on mobile.

Anyhow, modulo double checking licensing, lgtm.

I do think this is exactly the sort of thing I would rather have in snippets than a particular pyiron module, but since snippets isn't published yet and moving it there will be trivial with my new gist, I have no problem putting it here for now if you don't want to wait.

@samwaseda
Copy link
Member Author

I do think this is exactly the sort of thing I would rather have in snippets than a particular pyiron module, but since snippets isn't published yet and moving it there will be trivial with my new gist, I have no problem putting it here for now if you don't want to wait.

I agree and exactly for the reason that it’s not published I kept it here.

@samwaseda samwaseda merged commit 37a3d90 into main May 25, 2024
25 checks passed
@samwaseda samwaseda deleted the replace_deprecated_disutils branch May 25, 2024 19:30
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.

None yet

3 participants