-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep original line endings when reading expectation files #25
base: master
Are you sure you want to change the base?
Conversation
0d31584
to
1d636b5
Compare
Could you please elaborate and gimme some more details and/or an example? I just can't understand what problem you're trying to solve %) |
While I was working on #24, I noticed that regular tests (not from the previous issue) with
Here’s a quick demo of what’s happening: # test: 00000000: 410d 0a42 A..B
# \r \n
with open("test", "w") as f:
f.write("A\r\nB")
with open("test", "r") as f:
print(repr(f.read())) # “A\nB” While this works as desired: with open("test", "r", newline=””) as f:
print(repr(f.read())) # “A\r\nB” |
src/matcher/plugin.py
Outdated
@@ -101,7 +101,8 @@ def _maybe_store_pattern(self, text: str) -> None: | |||
self._pattern_filename.parent.mkdir(parents=True) | |||
|
|||
# Store! | |||
self._pattern_filename.write_text(text) | |||
with self._pattern_filename.open('w', newline='') as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this one is unnecessary and needs to be removed: the conversion of newlines doesn't happen during writing (see https://peps.python.org/pep-0278/#specification).
46ffe41
to
2991c3e
Compare
What makes me worried is that this will ruin (almost) everything! Indeed, for example, I have a trivial output matching test If I needed smth system-specific, I add a Obviously, this patch will make this way broken... I've taken a closed look at the test added by the #24 ... it looks incorrect to me %) |
Hmm, indeed, that’s something I hadn’t considered. My initial thoughts were that expectation texts should be treated as immutable, without any changes, and files are just an intermediate state of them, but, yes, Git couldn't be excluded from the story. |
I recently came across a binary file with an ASCII text header that needed to be parsed, and the header contains Windows-style line endings. The imaginary test case (just for an example) for some header extract function would be to output the header as is. This is not a platform-specific case. As I understand, on Unix, it’s not possible to have a pattern with CRLF symbols right now (even if we add So, the only way is to read it in a binary mode? |
Let's discuss it in #24, I'll answer there. |
I didn't get it... why not? The other question is if you want to make this test run on all platforms (w/ different native EOLs) and a test's input data file is "static" (in terms of EOL style in its beginning (I'm thinking about some "self-extract" archive w/ a shell script at the beginning %) obviously u need to preserve EOLs in the pattern file added to VCS (git I guess. so What could go wrong here? %) |
0196775
to
68dc622
Compare
The problem is that the test from the PR doesn't pass without the proposed changes because of the missing CR symbol. This may look unexpected and confusing for users, since an expectation file actually contains the symbol, but not the fixture. |
Changes in this PR
Since Python by default opens files in the universal newlines mode (
newline=None
), line ending characters are translated. Which is not good for us. So, let's preserve original line endings while writing and reading expectation files.