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

Issue 229: Add explicit encoding, license and unix line separators. #231

Merged
merged 1 commit into from
Dec 26, 2018

Conversation

erozqba
Copy link
Collaborator

@erozqba erozqba commented Dec 5, 2018

Fixes #229

Changes proposed in this pull request:

Some maintainers for different Linux distribution are having some
problems with the encoding in different python versions. This change
intents to make all python files define explicitly the encoding, license,
and Unix line separators. Remove one example of the README.rst file
where a possible non UTF-8 character is used.

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

Make sure all tests pass and coverage remains the same.

@null-git
Copy link

null-git commented Dec 6, 2018

Thanks for this, but it doesn't build either:

===>  Configuring for py36-num2words-0.5.8_1
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "setup.py", line 59, in <module>
    version=find_version("bin/num2words"),
  File "setup.py", line 47, in find_version
    for line in fp:
  File "/usr/local/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 747: ordinal not in range(128)
*** Error code 1

This is the same example in bin/num2words:27 that was removed from README.rst. Removing this line as well does the trick. But wouldn't it be nicer to just open those specific files as utf-8? As they are opened by open(), the "coding: utf-8" line doesn't do anything for this. Replacing open() with io.open() might do it? This seems to be available for python2.7 as well and to be the same as open() in python3+

@erozqba
Copy link
Collaborator Author

erozqba commented Dec 7, 2018

@null-git I agree! I will also use the io.open(..., encoding="utf-8")

I have updated my branch, could you give it a try again?

@null-git
Copy link

This builds fine now for with python 3.6 but you've used encoding='utf-8' for open() instead of io.open(). This should fail for python 2.7.

setup.py Show resolved Hide resolved
@null-git
Copy link

Sorry, I completely missed that. I think it's fine how you did it, other people won't care to much which open() this is and as python 2.7 is hopefully gone soonish anyway, both functions will be the same for most people anyway. Looks good to me.

Copy link
Contributor

@pierreduchemin pierreduchemin left a comment

Choose a reason for hiding this comment

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

Hi @erozqba,
Here are just a few basic questions before merge.
LGTM otherwise.

bin/num2words Outdated Show resolved Hide resolved
num2words/lang_UK.py Outdated Show resolved Hide resolved
num2words/utils.py Outdated Show resolved Hide resolved
num2words/lang_SL.py Show resolved Hide resolved
@erozqba erozqba force-pushed the issue_229 branch 2 times, most recently from a99feaa to 8b15fbc Compare December 23, 2018 16:49
@coveralls
Copy link

coveralls commented Dec 23, 2018

Pull Request Test Coverage Report for Build 601

  • 556 of 601 (92.51%) changed or added relevant lines in 10 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.579%

Changes Missing Coverage Covered Lines Changed/Added Lines %
num2words/lang_UK.py 58 59 98.31%
num2words/lang_TH.py 102 105 97.14%
num2words/lang_HE.py 17 58 29.31%
Totals Coverage Status
Change from base Build 595: 0.0%
Covered Lines: 4741
Relevant Lines: 5027

💛 - Coveralls

Some maintainers for different linux distribution are having some
problems with the encoding on different python versions. This change
intents to make all python files define explicitly the encoding, license
and unix line separators. Remove one example of the README.rst file
where a possible non UTF-8 character is used.
@erozqba
Copy link
Collaborator Author

erozqba commented Dec 23, 2018

@pierreduchemin Please review this PR again, it should be ready. Please review carefully the files on which I have changed the line end. Thanks a lot.

Copy link
Contributor

@pierreduchemin pierreduchemin left a comment

Choose a reason for hiding this comment

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

Carefully reviewed.
LGTM

@pierreduchemin pierreduchemin merged commit c12131d into master Dec 26, 2018
@pierreduchemin pierreduchemin deleted the issue_229 branch December 27, 2018 14:30
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.

None yet

4 participants