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

Update to check for proper UTF8 on Windows #2174

Merged
merged 1 commit into from
Nov 1, 2021

Conversation

Gizmokid2005
Copy link
Contributor

The locale.getlocale() check currently looks for "UTF-8" only, however on windows with powershell and UTF8 enabled the return value is "utf8" so this check still fails.

Description

Fixes #2173

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

@Exirel
Copy link
Contributor

Exirel commented Aug 16, 2021

Before I approve, note that we usually use sopel: [message] instead of __init__.py for the commit message.

I was tempted to dismiss the PR because we don't officially support Windows (someone would have to actually test Sopel on Windows, and we don't do that), however the fix is rather simple, and shouldn't be a problem.

Also, I'm not even sure that this warning is legitimate anymore, as I don't recall Python 3.6+ doing anything out of the ordinary when the system is not utf-8. I mean... unless someone did something really wrong with stdout...

@Gizmokid2005
Copy link
Contributor Author

Before I approve, note that we usually use sopel: [message] instead of __init__.py for the commit message.

I was tempted to dismiss the PR because we don't officially support Windows (someone would have to actually test Sopel on Windows, and we don't do that), however the fix is rather simple, and shouldn't be a problem.

Also, I'm not even sure that this warning is legitimate anymore, as I don't recall Python 3.6+ doing anything out of the ordinary when the system is not utf-8. I mean... unless someone did something really wrong with stdout...

I misunderstood the contributing docs when I made the commit then, my apologies.

Make your commit messages clear and explicative. Our convention is to place the name of the thing you're changing in at the beginning of the message, followed by a colon: the plugin name for plugins, "docs" for documentation files, "coretasks" for coretasks.py, "db" for the database feature, etc.

I normally don't use windows for things, but was trying to setup a quick touch dev environment and was running into this. I'm not sure of the validity of the Python warning though.

@dgw
Copy link
Member

dgw commented Oct 31, 2021

Hopping around through some blog posts from random people eventually led me to find PEP 540, which talks about assuming UTF-8 for the C locale on Unix starting in Python 3.7. I've reached my limit for text encoding nonsense today, but it sounds like we do still need this check at present.

Python under Windows also allegedly uses different encoding modes depending on how it's run (lost the source link for this one mid-rabbit-hole, sadly), but we won't concern ourselves with that one.

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

I would just amend the commit message myself, but since this commit is signed I'm sure @Gizmokid2005 would prefer to do it and keep the lovely green badge. Our usual format for this commit message would look like:

sopel: Update to check for proper UTF8 on Windows

The locale.getlocale() check currently looks for "UTF-8" only, however
on windows with powershell and UTF8 enabled the return value is "utf8"
so this check still fails.

@dgw dgw added this to the 7.1.6 milestone Nov 1, 2021
@dgw dgw changed the title __init__.py: Update to check for proper UTF8 on Windows Update to check for proper UTF8 on Windows Nov 1, 2021
@dgw dgw added Bugfix Generally, PRs that reference (and fix) one or more issue(s) Windows labels Nov 1, 2021
@Gizmokid2005
Copy link
Contributor Author

I would just amend the commit message myself, but since this commit is signed I'm sure @Gizmokid2005 would prefer to do it and keep the lovely green badge. Our usual format for this commit message would look like:

sopel: Update to check for proper UTF8 on Windows

The locale.getlocale() check currently looks for "UTF-8" only, however
on windows with powershell and UTF8 enabled the return value is "utf8"
so this check still fails.

Thanks! I'll get the commit amended today.

The locale.getlocale() check currently looks for UTF-8 only, however
on windows with powershell and UTF8 enabled the return value is utf8
so this check still fails.
@Gizmokid2005
Copy link
Contributor Author

@dgw - Sorry about the noise (this is actually a first time I've amended a commit and well...Monday didn't go smoothly. Should be good now, let me know if you need anything else from me!

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

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

Oops, we lost the green "Verified" badge anyway, but it sounds like you probably learned something about amending. Not a total loss? 😸

@dgw dgw merged commit 710a20d into sopel-irc:master Nov 1, 2021
dgw added a commit that referenced this pull request Nov 1, 2021
Update to check for proper UTF8 on Windows

Corresponds to (from master): 710a20d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix Generally, PRs that reference (and fix) one or more issue(s) Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UTF-8 Check fails on windows in Powershell
4 participants