-
-
Notifications
You must be signed in to change notification settings - Fork 301
Add Unicode Support #629
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
Add Unicode Support #629
Conversation
2133c83 to
7234936
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@ ## master #629 +/- ## ========================================== + Coverage 97.31% 97.33% +0.02% ========================================== Files 42 42 Lines 2045 2104 +59 ========================================== + Hits 1990 2048 +58 - Misses 55 56 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adam-grant-hendry Thanks for the update! Just one minor discussion needed on my side.
@woile I'm planning on merging this next week. Let me know if you need more time to take a deeper look. Thanks!
LGTM, the only thing missing is documentation: https://commitizen-tools.github.io/commitizen/config/
Ideally, I think the cz init should ask for the encoding if you are on windows, this would make windows users aware of potential encoding issues
It seems CI failed on windows 🤔
8c30aee to
5136163
Compare
@Lee-W I didn't push any commits yesterday, but it is showing that I did through Unverified commits. By chance, was this you?
Hi @adam-grant-hendry , yes I tried to rebase the master branch to this one to see if the latest update fix this branch
Is this still valid? Please rebase 🙏🏻
5136163 to
f028055
Compare
@woile @Lee-W Apologies for taking so long: I've been away for quite some time. My gpg signature keys are outdated, but the commits are by me.
mypy is still unhappy with line 6 of out.py (See: python/typeshed#3049). I added a #type: ignore, but Python 3.7 reads this as an unused type ignore (See: https://github.com/commitizen-tools/commitizen/actions/runs/5491623964/jobs/10008334198#step:5:251), so I'm stuck.
Only remaining item is to add documentation. I added encoding as a keyword argument to changelog.py/get_metadata so as to not break backwards-compatibility with the added scheme argument.
@adam-grant-hendry Sure! I'll take a look these days.
Hi @adam-grant-hendry , sorry for the late review. Overall the changes are good, but I think we should drop commit
3430edba7a5b1b4f59e5b7aa48e03bd15ccc32a4and9af6c748eb1084c5239e8d8058e7eddd59abbfb5which downgrades the version of our GitHub actions workflow. Also, I notice someopenandsmart_openmiss thisencodingfeature. Is this intentional? Or just accidentally missed? Thanks!
No worries at all. Thank you for your review!
- Yes, good find: we should definitely drop those commits then
- That was certainly an accidental miss on my part. I’ll review, make changes, and push up the fixes
I probably can’t get to this today, but I can start tomorrow. I’ll see if I can request your re-review by Monday morning.
Thanks again!
e08d0d5 to
3383821
Compare
@Lee-W I made all the changes discussed. Please re-review this PR at your earliest convenience. Thank you!
@adam-grant-hendry Sure! I'll try to take a look this weekend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I'm good with this PR. But we missed a few places missed. Wondering is it designed this way or just missed.
commitizen/commitizen/commands/init.py
Line 38 in 114501d
commitizen/hooks/prepare-commit-msg.py
Line 63 in 114501d
But, yep looks like they might be something not affected 🤔
I'm planning on merging this next week. @woile Please let me know if you want to take a deeper looks 🙂
@adam-grant-hendry ah, just notice a conflict. might need your help to resolve it.
...But we missed a few places missed. Wondering is it designed this way or just missed.
commitizen/commitizen/commands/init.py
Line 38 in 114501d
with open("pyproject.toml") as f:,
commitizen/hooks/prepare-commit-msg.py
Line 63 in 114501d
with open("/dev/tty") as tty:.
But, yep looks like they might be something not affected 🤔
I purposefully left these out as I considered them "trivial" cases. By that I mean the "pyproject.toml" case is only looking for the "[tool.poetry]" table heading, which should generally not be affected by whether the file is opened in utf-8 mode or not. As for "/dev/tty", there is no equivalent to "/dev/tty" on Windows machines, so this would only run properly from a Linux environment, in which case the encoding is already utf-8 by default.
Sounds good 👍 Just want to confirm it. I think we're good to merge it after resolving the conflict. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well. Thanks!
8ae25d0 to
69c90ab
Compare
This will allow commiting, e.g., emoji's and parsing commit messages for unicode characters when creating change logs.
Also, use `utf-8` by default on Windows in `out.py`.
Map `"encoding"` and `"name"` to `encoding` and `name` variables, respectively (removes hard-coding of values).
Add `encoding` parameter to `open` and `smart_open` method calls where missed. Use `defaults.encoding` as default.
093eb23 to
ecc3365
Compare
@adam-grant-hendry ah, just notice a conflict. might need your help to resolve it.
I had to pull in the latest commits to master and rebase. It's all good to go now!
Many thanks! Just merged
Uh oh!
There was an error while loading. Please reload this page.
Adds unicode support by allowing configurable encodings to be specified (defaults to
utf-8).Checklist
./scripts/formatand./scripts/testlocally to ensure this change passes linter check and testFixes: Issue #516