remove .decode('utf-8') for python3 support

arthepsy / ssh-audit

SSH server auditing (banner, key exchange, encryption, mac, compression, compatibility, security, etc)

MIT License

2.96k stars 269 forks source link

remove .decode('utf-8') for python3 support #15

Closed yusufsyaifudin closed 7 years ago

yusufsyaifudin commented 7 years ago

this PR intended to fix this issue #14

As mentioned by @Rogdham, I think that we should removed .decode('utf-8') because "Begin with Python 3, all string is unicode object.", based recommendation from answer for this question and answer for this question, it should be removed.

note: I've tested running the test py.test --cov-report= --cov=ssh-audit -v test and python3 -m py.test --cov-report= --cov=ssh-audit -v test for python3 and have no error.

But, double check should be done because I don't know what the reason behind of using .decode('utf-8'), maybe it to avoid strange behavior for special case (?).

coveralls commented 7 years ago

Coverage remained the same at 56.726% when pulling 8507064bef9c6e8a433db90414f210a486de42e2 on yusufsyaifudin:master into 76509a1011fa4da6644ec2a1627dc58853569378 on arthepsy:master.

arthepsy commented 7 years ago

Thanks for the work and references. You are right, that this bug got introduced to avoid strange case. But as this part of the code wasn't tested automatically (jet), it opened gates for other bug. For more information, read my comment in #14.

I don't think that it's the correct fix to remove .decode('utf-8'), because the source data is bytes and it should be decoded to get back unicode (Python 2) or str (Python 3).

Therefore, I fixed it a bit differently. Also, I added static type checks via (http://mypy-lang.org/) to be sure of the fix. In any case, can You verify that 6b76e68d0dab0701a833ee85663161278ef2b5d0 fixes the bug?

arthepsy commented 7 years ago

This and other encoding/decoding issues are fixed in 1.7.0. Also, multiple of tests added.