Skip to content

refactor(tool): use charset_normalizer instead of chardet#550

Merged
woile merged 1 commit intocommitizen-tools:masterfrom
gpongelli:use_charset_normalizer
Aug 5, 2022
Merged

refactor(tool): use charset_normalizer instead of chardet#550
woile merged 1 commit intocommitizen-tools:masterfrom
gpongelli:use_charset_normalizer

Conversation

@gpongelli
Copy link
Copy Markdown
Contributor

@gpongelli gpongelli commented Aug 5, 2022

Description

move from chardet to charset_normalizer to avoid recent issue on wrong encoding.

Checklist

  • Add test cases to all the changes you introduce
  • Run ./scripts/format and ./scripts/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

Expected behavior

commitizen is able to decode command's strings without issue on decode .

Steps to Test This Pull Request

Additional context

#545

#544

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 5, 2022

Codecov Report

Merging #550 (37505ef) into master (ed636ba) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #550   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files          39       39           
  Lines        1556     1556           
=======================================
  Hits         1529     1529           
  Misses         27       27           
Flag Coverage Δ
unittests 98.26% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/__version__.py 100.00% <100.00%> (ø)
commitizen/cmd.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jenstroeger
Copy link
Copy Markdown
Contributor

In the future I’d appreciate it if people communicate the duplicated effort, so that I don’t have to waste my time writing a PR only so it will be replaced by another.

@woile
Copy link
Copy Markdown
Member

woile commented Aug 5, 2022

I think your change is still valid. It's worth changing the chardet library in this PR. But we can still have the same issue as before, or I'm missing something? Sorry for the inconvenience occasionated if so.

@gpongelli
Copy link
Copy Markdown
Contributor Author

Hi Jens, sorry but adding chardet comes from my PR and, after all the recent issues made by it, I felt unhappy so I’ve searched and tested an alternative.

I hope it works, and probably could be useful to add some more tests to avoid regression.

@Lee-W Lee-W mentioned this pull request Aug 7, 2022
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.

3 participants