Ticket #3481 (closed enhancement: fixed)

Opened 23 months ago

Last modified 14 months ago

Improve Trial's --temp-directory option

Reported by: thijs Owned by: thijs
Priority: high Milestone:
Component: trial Keywords:
Cc: Branch: branches/trial-tmp-marker-3481
Author: jonathanj, thijs Launchpad Bug:

Description

Trial should drop a marker file into _trial_temp and refuse to delete and re-create any directory that doesn't have the marker. Docs should updated as well, see #2589

Attachments

trial-marker.diff Download (2.2 KB) - added by jonathanj 16 months ago.
trial-marker-1.diff Download (4.4 KB) - added by jonathanj 15 months ago.

Change History

Changed 18 months ago by moonfallen

Yes, let's do this please. Just had my source directory deleted. Didn't lose anything important, fortunately.

Changed 16 months ago by jonathanj

Changed 16 months ago by jonathanj

  • keywords review added
  • owner jml deleted

Changed 15 months ago by glyph

  • owner set to jonathanj
  • keywords review removed

The patch is great! Could you make a few small adjustments?

  1. In your test case, the name "my_runner" violates the coding standard. It should be "myRunner".
  2. In _removeSafely's docstring (thanks for adding that!) you should probably use L{} rather than C{} to refer to NoTrialMarker.
  3. I realize that trial is a pile of bad examples as far as this particular request goes, but could you use twisted.python.filepath.FilePath rather than os.path for path inspection and manipulation? As a bonus (although this is not required) you could remove the dependence on the shutil module, since FilePath handles recursive removal itself.
  4. NoTrialMarker's docstring could be more expository. Especially, why is this exception raised? When does trial look for a marker?
  5. Also, you might want to follow the example of _WorkingDirectoryBusy and make the exception private, since the only method that raises it is also private. It's always easy to make private stuff public later, less easy to decide it was a bad idea and make it private.

Changed 15 months ago by jonathanj

Changed 15 months ago by jonathanj

  • keywords review added
  • owner changed from jonathanj to glyph
  1. I've changed _setUpTestdir and _removeSafely to use FilePath internally.

Everything else you said made sense, and I've addressed those changes too.

Changed 15 months ago by thijs

  • branch set to branches/trial-tmp-marker-3481
  • branch_author set to thijs

(In [26970]) Branching to 'trial-tmp-marker-3481'

Changed 15 months ago by thijs

(In [26972]) Update copyright, refs #3481

Changed 15 months ago by thijs

  • keywords review removed
  • branch_author changed from thijs to jonathanj, thijs

Buildbot is  happy, looks ready to merge for me.

If this hits the trunk before #2589 does, then that one should be closed, or marked as fixed with the merge of this ticket. #2589 was created to add a warning to trial's commandline options, but I don't think we need to add more docs than that.

Changed 14 months ago by glyph

  • owner changed from glyph to thijs

Erm... did you review this, or not? If so, then it should be reassigned to jonathanj, not me. If not, and you're looking for more feedback from me (although I don't see the question) as the reviewer of a non-committer contribution, you should merge it on behalf of jonathanj :).

Changed 14 months ago by glyph

Gah, that comment was badly garbled. What I meant was:

If you reviewed this, you should be merging it on behalf of jonathanj.

If you just looked at it and gave it a positive comment, but wanted feedback from me, then you should be leaving the review keyword as you reassign it to me.

Changed 14 months ago by thijs

  • status changed from new to closed
  • resolution set to fixed

(In [27098]) Merge trial-tmp-marker-3481: Trial now drops a marker file into _trial_temp and refuses to delete and re-create any directory that doesn't have the marker.

Author: jonathanj, thijs Reviewer: glyph Fixes: #3481

Note: See TracTickets for help on using tickets.