You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Wrap the releasing in a try/catch to avoid errors when Windows is being difficult. I did also put the resetting of the list of temporary directories outside the for loop. Don't think it actually matters, but looks better to me.
I added a parameter in the function that allows the CLI to always ask for the temporary directories to be released. This is something @bnmnetp has wanted, since he uses debug-level logging when building in runestone, but doesn't want all the temporary directories to clutter up his build server. Shouldn't make any difference when using the core pretext script.
Of course, it's up to you if you want to keep both or just the first one.
Thanks very much, @oscarlevin. Good catch on the reset of __temps to the empty list inside the loop! That was definitely not right. Claude and I both had things to say, so I just built a follow-on commit and let Claude write an "action report," coming next. Merged as-is.
Thanks @oscarlevin. I added a small follow-on commit from a review pass — summary below, as review item → disposition:
1. Required any_log_level breaks existing callers. Item: the new parameter has no default, so any caller of release_temporary_directories() that doesn't pass it (e.g. vendored copies) breaks; a default of False would keep them working. Decision:Left as-is, on purpose — we want consumers to notice and update. 😉
2. Wrong log level on successful removal. Item:log.warning("Removed temporary directory …") fires on every success — wrong level, and redundant with the preceding log.info. Change:dropped to log.debug.
3. Stale comment. Item:# conservatively, raise exception on errors no longer matched, since the new try/except catches that raise. Change:reworded to "let a removal failure raise, so it is caught and reported below".
4. Bare except, and the failure didn't say why. Item:except: also swallows KeyboardInterrupt/SystemExit, and the warning didn't include the cause. Change:now except Exception as e:, and the message reports it: … (and maybe some others): {}".format(td, str(e)).
5. __temps not emptied on partial failure. Item: if removal raises partway, the __temps = [] after the loop is skipped, so the global keeps both already-removed and remaining dirs — which a long-running host process (e.g. Runestone) could accumulate across builds. Change:moved the reset into a finally, so __temps is always emptied even when a removal raises.
6. Double blank line after the docstring — removed.
Claude Opus 4.8, acting as a review assistant for Rob Beezer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two commits:
Of course, it's up to you if you want to keep both or just the first one.