Add explicit encoding to read_text. #577
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jaraco
force-pushed
the
debt/encoding-warning
branch
2 times, most recently
from
734f1fb
to
238563a
Compare
graingert
approved these changes
Feb 10, 2024
|
Might be worth enabling encoding warnings |
|
Here's where I do that for my projects: I scanned through the equivalent config for towncrier (the noxfile), but I don't immediately see how a environment variable could be set. It's also not obvious to me if it should be set for tests only or for all sessions. Reading through the docs, the Interestingly, running |
|
Adding the environment variable to the tests does reveal that there are many more locations in the code where the warning is triggered: |
|
Here's a distilled list of the violations: |
|
82 checks is a little steep to fix by hand. I looked and ruff doesn't (yet) support fixing the warnings. How do you feel about enabling the warnings but not fixing all of them yet? |
jaraco
force-pushed
the
debt/encoding-warning
branch
from
5731419
to
d4ba19e
Compare
jaraco
force-pushed
the
debt/encoding-warning
branch
from
8396bac
to
dd70c55
Compare
7 tasks
adiroiban
approved these changes
Apr 28, 2024
| @@ -0,0 +1 @@ | |||
| Add explicit encoding to read_text. | |||
There was a problem hiding this comment.
I don't know if anyone is using non UTF-8 encoding in these days for source code.... maybe some legacy Windows shops might still use UTF-16
I am ok with this change, but I think that somewhere in the documentation, we should mention that towncrier can only work with UTF-8 fragment files and it will produce UTF-8 encoded files.
Maybe this can be done as part of the "Philosophy" section
https://towncrier.readthedocs.io/en/latest/#philosophy
Maybe add
It assumes that the source code is stored in UTF-8 files.
There was a problem hiding this comment.
I don't know if anyone is using non UTF-8 encoding in these days for source code.... maybe some legacy Windows shops might still use UTF-16
In my experience, that's not been the case, even for text-oriented applications like towncrier. My instinct is it's still a bug fix, but for Windows users, it is technically a breaking change. My advice - hope that there aren't any Windows users relying on locale but be prepared to back out the change and introduce a compatibility shim if someone does encounter an issue. I'm happy to help with that if needed.
Maybe this can be done as part of the "Philosophy" section
I'm a little uneasy adding it to the Philosophy section since that section comes from the readme, and it feels more like an implementation detail. Philosophy doesn't even mention markdown or restructured text, a concern that's going to be a lot more salient to a reader.
In 83b8c02, I've proposed that it be added to the tutorial, near where "markdown" is mentioned and shortly after the user has first encountered creating content for news fragments. Let me know what you think.
There was a problem hiding this comment.
In e1caa2c, I took it a step further and explicitly mentioned other concerns that expect UTF-8.
|
@jaraco with this PR, I can see that the import warning are gone. Please consider update the documentation to mention that UTF-8 is mandatory and I think that after that we can merge this. Thanks again and sorry for the delay |
jaraco
force-pushed
the
debt/encoding-warning
branch
from
c484029
to
2b9e1cf
Compare
7 tasks
|
Thanks for the changes. I have merged this. If someone wants other encoding type, they can send a report and a PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #561
Description
Checklist
src/towncrier/newsfragments/. Describe yourchange and include important information. Your change will be included in the public release notes.
docs/tutorial.rstis still up-to-date.docs/cli.rstreflects those changes.docs/configuration.rstreflects those changes.