Skip to content
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

Explicitly specify file encoding for windows #2007

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalito
Copy link

@dalito dalito commented Mar 3, 2025

It seems that this place was overlooked. The file encoding is specified on all other non-binary open(...) calls except for this one.

Closes #2006

Copy link
Member

@sisp sisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising this problem and offering a PR to fix it, @dalito! 🙏

I have an inline suggestion. And could you try adding a test that fails without the fix?

Comment on lines +1235 to 1238
with Path(fname).open(encoding="utf-8") as conflicts_candidate:
if any(
line.rstrip()
in {"<<<<<<< before updating", ">>>>>>> after updating"}
Copy link
Member

@sisp sisp Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or how about opening the file with "rb" mode and turning the markers into byte strings?

Suggested change
with Path(fname).open(encoding="utf-8") as conflicts_candidate:
if any(
line.rstrip()
in {"<<<<<<< before updating", ">>>>>>> after updating"}
with Path(fname).open("rb") as conflicts_candidate:
if any(
line.rstrip()
in {
b"<<<<<<< before updating",
b">>>>>>> after updating",
}

This way, we're independent of encodings.

@dalito
Copy link
Author

dalito commented Mar 4, 2025

@sisp Can you point me to the approximate place where to add a test?

Ah, I just remember this: It may not be possible to reproduce this on the Windows runner as the encoding is different than on local Windows10/11.

@sisp
Copy link
Member

sisp commented Mar 4, 2025

@sisp Can you point me to the approximate place where to add a test?

I'd add a test at the bottom of https://github.com/copier-org/copier/blob/master/tests/test_updatediff.py.

Ah, I just remember this: It may not be possible to reproduce this on the Windows runner as the encoding is different than on local Windows10/11.

We can mock the default encoding. See, e.g., https://github.com/copier-org/copier/pull/2003/files#diff-7e70d7a9d4f971464d4b2f4f2bb1c5154cd6de349132cdf4acafc3f28b05bf4d. Does this help?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnicodeDecodeError when updating on Windows
2 participants