Several small improvements#38
Merged
Merged
Conversation
added 3 commits
February 27, 2026 12:05
This patch removes unused legacy attributes of which it is unclear why they were preserved in the first place.
This patch introduces a helper function _path.non_existent to deduplicate code found in both the html logger and the data logger.
joostvanzwieten
requested changes
Feb 27, 2026
| except FileExistsError: | ||
| if not exist_ok: | ||
| raise | ||
| except FileNotFoundError: |
Member
There was a problem hiding this comment.
FDPath(dir_fd, '').mkdir(parents=True) recurses indefinitely, because os.mkdir('', mode, dir_fd=dir_fd) raises FileNotFoundError. I think we should stop the recursion when self.path is empty.
| return path | ||
| dir_fd = os.open(path, flags=os.O_RDONLY) | ||
| return _FDDirPath(dir_fd) | ||
| return FDPath(dir_fd, '') |
Member
There was a problem hiding this comment.
It looks like we're now leaking the fd. _FDDirPath called os.close() in the destructor.
Contributor
Author
There was a problem hiding this comment.
Good point. I pulled the commit because this change was anyhow not relevant for this PR (I just thought let's merge what we can already from #37).
added 6 commits
February 27, 2026 15:38
This patch adds a file parameter to the StdoutLog, defaulting to sys.stdout, and removes the StderrLog in favour of a StdoutLog with file=sys.stderr. The expectation is the StderrLog is never used and that it can be removed without warning. The unit tests are changed because the capture function relied on contextlib.redirect_stdout and contextlib.redirect_stderr, which no longer has effect if the logger has a handle to the file object. The new strategy is to create a StdoutLog with a StringIO argument and read out the value afterward. This has the important advantage of of leaving sys.stdout intact so that error messages can be read if a test fails.
This patch removes the Log base class and instead derives test cases directly from unittest.TestCase to make their workings less obscure. Most tests defer work to a check_output method with the aim that they can also be called externally by RecordLog and TeeLog.
This patch simplifies the StdoutLog, LoggingLog and RichOutputLog classes by incorporating the pushcontext, popcontext and recontext methods from the ContextLog base class. The ContextLog is removed for no longer serving a purpose.
This patch changes the StdoutLog writer to indent multi-line messages as
follows:
some > context > this is a message that
> spans two lines
This patch changes the RichtOutputLog writer to indent multi-line
messages as follows:
some > context > this is a message that
> spans two lines
gertjanvanzwieten
force-pushed
the
next
branch
from
75e99f4 to
aae4411
Compare
joostvanzwieten
approved these changes
Feb 27, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.