AcademySoftwareFoundation / rez

An integrated package configuration, build and deployment system for software
https://rez.readthedocs.io
Apache License 2.0
932 stars 329 forks source link

Add utility functions to rename a file or folder src to dst with retrying. #1666

Closed loonghao closed 5 months ago

loonghao commented 6 months ago

This PR is try to fix #1665

JeanChristopheMorinPerso commented 6 months ago

Thanks @loonghao for submitting this PR. My main concern right now is speed. Starting a subprocess on Windows usually adds a noticeable overhead.

Could the code first try to os.rename and only fallback to the subprocess method if it's running on windows and the error is "WinError 5" or something like that?

Also, it would be great if you could add tests to cover all cases covered by the change.

codecov[bot] commented 6 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 58.03%. Comparing base (91db537) to head (dafcd6c).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1666 +/- ## ========================================== + Coverage 57.99% 58.03% +0.03% ========================================== Files 128 128 Lines 17088 17102 +14 Branches 3502 3504 +2 ========================================== + Hits 9911 9925 +14 Misses 6509 6509 Partials 668 668 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

loonghao commented 6 months ago

@JeanChristopheMorinPerso I have updated the code and added a unit test for this function. Please take another look. Thanks.

loonghao commented 5 months ago

Thanks! There is a bug in the code and I think we can add more tests.

@JeanChristopheMorinPerso I have updated the code and added more unit tests. Please take another look.

Thanks.

JeanChristopheMorinPerso commented 5 months ago

Thanks @loonghao ! This should hopefully be my last batch of comments.

loonghao commented 5 months ago

Thanks @loonghao ! This should hopefully be my last batch of comments.

@JeanChristopheMorinPerso I have made all the changes based on your feedback and addressed all the comments. Could you please take another look. Thanks!