Closed Bug 859422 Opened 7 years ago Closed 5 years ago
document why mozfile
.remove works better than shutil .rmtree
https://github.com/mozilla/mozbase/blob/master/mozfile/mozfile/mozfile.py#L95 Our docstring: """Removes the specified directory tree This is a replacement for shutil.rmtree that works better under windows.""" # (Thanks to Bear at the OSAF for the code.) We should document: * why it works better under windows ** including the precise errors that can be encountered via windows with shutil.rmtree that are avoided here * what errors mozfile.rmtree does *not* fix on windows (that work on other platforms) ** and file bugs on them * and of course we should have tests illustrating all of this https://bugzilla.mozilla.org/show_bug.cgi?id=859178#c1
See Also: → 859426
see also https://bugzilla.mozilla.org/show_bug.cgi?id=860426 and https://bugzilla.mozilla.org/show_bug.cgi?id=861271
See Also: → 860426
mozfile.rmtree() is depricated in favor of mozfile.remove() and will be removed in the future. So updating the summary.
Summary: document why mozfile.rmtree works better than shutil.rmtree → document why mozfile.remove works better than shutil.rmtree
I've updated the documentation and added one test case. Some related tests were already present in mozbase/mozfile/tests/test_remove.py. Unfortunately I don't know what errors mozfile.rmtree does *not* fix on windows, so this part may be not fixed.
Comment on attachment 8503623 [details] [diff] [review] document why mozfile.remove works better than shutil.rmtree I am no longer at Mozilla and so am not comfortable reviewing this change. Superficially, it looks good
Ok Jeff, thank you. :)
Posted this again to ask review by William this time.
Comment on attachment 8503630 [details] [diff] [review] document why mozfile.remove works better than shutil.rmtree Review of attachment 8503630 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. I assume you ran the unit tests to make sure they pass? If so, please say so and mark this as "checkin-needed".
Attachment #8503630 - Flags: review?(wlachance) → review+
I ran the tests on my own laptop but not pushed to try (never done that yet). I'm not sure either there is a push try for mozbase, looking at http://trychooser.pub.build.mozilla.org/. So I'll add the keyword. Please tell me if a push try is needed, and maybe if you have some time help me on this.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Shouldn't we also update the documentation itself? Less people will check the docstrings in the Python code, but http://mozbase.readthedocs.org/en/latest/mozfile.html is still the first place to look. And there we do not even have .remove() but still .rmtree() :/ So I checked more and I think we only have to change the following line to replace the former method: http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/docs/mozfile.rst#9 Julian, mind updating it so that the documentation is completely done?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I didn't notice the docs folder yet containing sphinx dox sources; Do you want another patch, or should I fix and replace the first one ? Also, is a review necessary here for that (assuming I only add the new one line patch) ? Thanks Henrik !
Given that the patch already landed, a follow-up patch would be fine. Not sure how patches for docs are handled in mozbase, so maybe better ask for review from William or Ahal.
Comment on attachment 8504118 [details] [diff] [review] update sphinx documentation for mozfile Review of attachment 8504118 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Thanks for the update, and thanks :whimboo for noticing that the documentation needed this change.
Attachment #8504118 - Flags: review?(wlachance) → review+
This is a documentation only change, so we can probably mark this as DONTBUILD if you want to save a few cycles.
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
William, do we have to manually trigger an update to refresh the docs at http://mozbase.readthedocs.org/en/latest/mozfile.html?
(In reply to Henrik Skupin (:whimboo) from comment #19) > William, do we have to manually trigger an update to refresh the docs at > http://mozbase.readthedocs.org/en/latest/mozfile.html? Annoyingly, yes, you do need to do this because we have no post-commit hook set up. I kicked off a new run, for future reference you should be authorized to do the same (I see your name in the owners list)
You need to log in before you can comment on or make changes to this bug.