Closed
Bug 859422
Opened 11 years ago
Closed 10 years ago
document why mozfile.remove works better than shutil.rmtree
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: k0scist, Assigned: parkouss)
References
Details
Attachments
(2 files, 1 obsolete file)
2.67 KB,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
791 bytes,
patch
|
wlach
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•11 years ago
|
||
See also https://bugzilla.mozilla.org/show_bug.cgi?id=859426
See Also: → 859426
Reporter | ||
Comment 2•11 years ago
|
||
see also https://bugzilla.mozilla.org/show_bug.cgi?id=860426 and https://bugzilla.mozilla.org/show_bug.cgi?id=861271
See Also: → 860426
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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.
Attachment #8503623 -
Flags: review?(k0scist)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → j.parkouss
Reporter | ||
Comment 5•10 years ago
|
||
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
Attachment #8503623 -
Flags: review?(k0scist)
Assignee | ||
Comment 6•10 years ago
|
||
Ok Jeff, thank you. :)
Assignee | ||
Comment 7•10 years ago
|
||
Posted this again to ask review by William this time.
Attachment #8503623 -
Attachment is obsolete: true
Attachment #8503630 -
Flags: review?(wlachance)
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c0d968dc9a29
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 11•10 years ago
|
||
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 → ---
Assignee | ||
Comment 12•10 years ago
|
||
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 !
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8504118 -
Flags: review?(wlachance)
Comment 15•10 years ago
|
||
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+
Comment 16•10 years ago
|
||
This is a documentation only change, so we can probably mark this as DONTBUILD if you want to save a few cycles.
Keywords: checkin-needed
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/575cbb47203d
Keywords: checkin-needed
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/575cbb47203d
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla35 → mozilla36
Comment 19•10 years ago
|
||
William, do we have to manually trigger an update to refresh the docs at http://mozbase.readthedocs.org/en/latest/mozfile.html?
Flags: needinfo?(wlachance)
Comment 20•10 years ago
|
||
(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)
Flags: needinfo?(wlachance)
You need to log in
before you can comment on or make changes to this bug.
Description
•