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

Fix 'character maps to <undefined>' issue on windows #22

Merged
merged 1 commit into from
Jul 27, 2021

Conversation

alx00x
Copy link
Contributor

@alx00x alx00x commented Jul 25, 2021

Fixes #21

@raethlein
Copy link
Member

raethlein commented Jul 26, 2021

Hey @koaleksa, thanks a lot for the PR and fixing the issue! I was just wondering whether this could break the behavior in certain cases / for certain users where the markdown file was not saved as 'utf-8'? I think this is still much better than leaving it out as in the original, though. But maybe we should make the default 'utf-8' overridable via a CLI argument? Do you have any thoughts about this?

@alx00x
Copy link
Contributor Author

alx00x commented Jul 27, 2021

Hi @raethlein, there are probably very few reasons why one would want to write a text file (markdown in particular) with any other encoding these days. An argument could be made, of course... If there are such users, the code will still run for them as function simply opens the file in write mode and replaces the content, but the encoding will indeed change for them under their feet.

I think, for a lightweight tool, I'd advocate for keeping things simple and adding options only when there's specific expressed interest, rather than going by unlikely scenarios. However, a CLI override would be trivial to implement so if you'd still prefer to expose the option, I'll be happy to amend this PR.

Thank you guys for the awesome tool, btw. 👍

For some interesting read on this topic, I suggest having a look at PEP597

@raethlein
Copy link
Member

Thanks a lot for your detailed thoughts and I totally agree with your assessment! I am going to merge it then :)

@raethlein raethlein merged commit ac4f681 into ml-tooling:main Jul 27, 2021
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.

Default encoding issue on windows
2 participants