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

When reading config files, require them to be encoded with UTF-8. #1735

Merged
merged 6 commits into from Apr 5, 2019

Conversation

Projects
None yet
4 participants
@jaraco
Copy link
Member

commented Apr 5, 2019

Closes #1702

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@jaraco jaraco force-pushed the bugfix/1702-utf8-config branch from d23391d to e89509f Apr 5, 2019

@benoit-pierre

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Shouldn't the setuptools.unicode_utils.detect_encoding helper be removed?

@jaraco jaraco requested a review from benoit-pierre Apr 5, 2019

@jaraco

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@pganssle I'd like to merge and release this today, so if you have any reservations, please let me know.

@pganssle

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I don't have a major problem with the idea. My over-cautious instinct is to deprecate first, but this is such an easy thing to fix if it breaks something that maybe it's not worth it?

(Famous last words)

@jaraco

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Good point. Here's my thinking: because the behavior was only recently released (January), probably not many project depend on the behavior or those that do are likely already cutting-edge enough to rely on this latest release. Moreover, if we find that releasing this does cause problems, I'll be on standby to release a bugfix release to add backward compatibility if needed (so only a small window of the problemmatic release).

@pganssle

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Oh if you are reversing a recently introduced behavior then I think it's even less likely to cause problems, I say go for it.

@jaraco jaraco removed the request for review from benoit-pierre Apr 5, 2019

@jaraco

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

Thanks both. In the interest of progress, I'm going to move forward with this, but don't hesitate to raise other concerns or thoughts.

@jaraco jaraco merged commit 4edd0d5 into master Apr 5, 2019

7 checks passed

codecov/patch 93.1% of diff hit (target 82.74%)
Details
codecov/project 82.75% (+0.01%) compared to 393809a
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@jaraco jaraco deleted the bugfix/1702-utf8-config branch Apr 5, 2019

@webknjaz

This comment has been minimized.

Copy link
Contributor

commented on setuptools/tests/test_setopt.py in b336e83 Apr 7, 2019

Did you use an online transliteration service? 🤣

(nevermind, I saw it's fixed in the next commit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.