Bug in result collector#60
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the result collector related to encoding and buffer handling in the LocalFileHandler class. The changes ensure consistent UTF-8 encoding across file operations and properly manage the internal buffer state during pull operations.
Changes:
- Added explicit UTF-8 encoding to file operations in
LocalFileHandlerandResultCollector - Modified
LocalFileHandler._pull()to properly reset buffer state before writing content - Added two new test cases to verify correct parsing behavior with the updated encoding handling
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| fourinsight/engineroom/utils/_core.py | Added encoding parameter to file operations and buffer management in LocalFileHandler._pull() and ResultCollector methods |
| tests/test_core.py | Added two test cases (test_error_parsing and test_error_parsing2) to validate the encoding fix |
| tests/testdata/drio_sdk_usage_mod.csv | New test data file with UTF-8 characters and edge cases |
| tests/testdata/drio_sdk_usage_mod2.csv | Additional test data file with UTF-8 characters for validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Note to self, check if this fix also applies to AzureBlobHandler |
Says in the readme it should work, but does not due to missing dep
|
Issue seems to be related to the stream truncating number of characters, but truncate is in bytes, not characters, so when we have UTF-8 with two bytes, we start truncating from the end and loose data |
| return f"LocalFileHandler {self._path.resolve()}" | ||
|
|
||
| def _pull(self): | ||
| return self.write(open(self._path, mode="r").read()) |
There was a problem hiding this comment.
This return that was removed, made the truncate function in the base class truncate differently.
|
The one failing test missing is due to not finding the file is meant to leave something untouched, I think. I guess we could check that the source exists before we truncate the stream or something. |
| else: | ||
| self.truncate(characters_written) | ||
| self.flush() | ||
| pos = self.tell() |
There was a problem hiding this comment.
We can just use tell to ask the stream where we are, and use that for truncation. I don't fully understand it but I think the flush is important so that the underlying buffer has been written to fully.
There was a problem hiding this comment.
It's possible the flush isn't necessary.
There was a problem hiding this comment.
Better safe than sorry, I'll rather flush one more too often in this case.
There was a problem hiding this comment.
Yup, it's probably unnecessary in case of files, but reading via streaming from Azure, there it might be important.
| self.truncate(characters_written) | ||
| self.flush() | ||
| pos = self.tell() | ||
| self.truncate(pos) |
There was a problem hiding this comment.
ah, smart
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| content = f.read() | ||
| self.seek(0) | ||
| self.truncate(0) | ||
| self.write(content) |
There was a problem hiding this comment.
This is where the return was lost
|
[like] Heidi Gryteland Holm reacted to your message:
________________________________
From: Bjørn Einar Bjartnes ***@***.***>
Sent: Wednesday, January 28, 2026 1:14:56 PM
To: 4Subsea/fourinsight-engineroom-utils-python ***@***.***>
Cc: Heidi Gryteland Holm ***@***.***>; Author ***@***.***>
Subject: Re: [4Subsea/fourinsight-engineroom-utils-python] Bug in result collector (PR #60)
@bjorn-einar-bjartnes-4ss commented on this pull request.
________________________________
In fourinsight/engineroom/utils/_core.py<#60 (comment)>:
@@ -143,11 +143,16 @@ def __repr__(self):
return f"LocalFileHandler {self._path.resolve()}"
def _pull(self):
- return self.write(open(self._path, mode="r").read())
+ # return self.write(open(self._path, mode="r", encoding=self.encoding).read())
+ with open(self._path, mode="r", encoding=self.encoding) as f:
+ content = f.read()
+ self.seek(0)
+ self.truncate(0)
+ self.write(content)
This is where the return was lost
—
Reply to this email directly, view it on GitHub<#60 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ANDDVFJGGFNWVAZDGCPKZND4JCY5BAVCNFSM6AAAAACS2YP2YOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTOMJWGY2DSOBSGI>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
Added consistent encoding in the LocalFileHandler, and changed LocalFileHandler pull buffer handling