#578 cli: fix changelog encode for PY3
Merged by mikem. Opened by julian8628.
julian8628/koji changelog-encode  into  master

Download 578.patch

I'm not sure if there is some other reasons to invoke str.encode('utf-8') here, So adding a wrapper to make sure only str be printed here

:thumbsup:

Why not just use fixEncoding? Your fixPrint seems to do essentially the same thing.

I not sure this updated comment is any more correct.

Why not just use fixEncoding? Your fixPrint seems to do essentially the same thing.

for Python3, fixEncoding will return bytes which will be directly printed as b'\xe4\xbd\xa0\xe5\xa5\xbd'
fixPrint is supposed to make sure the bytes decoded to '你好' to fix above issue.
I think a better choice should be to make sure fixEncoding to always return str, but I'm not sure if it will introduce other problems.

wrote a candidate modifying fixEncoding and fixEncodingRecurse here:
https://pagure.io/fork/julian8628/koji/c/3d55e9d

~~I cannot replicate #577 with client HEAD (or from koji-1.13.0 release). Can you?~~

Nevermind, got it

Interestingly, fixEncoding is barely used client side. It's used several places in the hub, web, and builder, but in the client lib the only place is in parse_pom (which itself is mostly used on hub/builder, but is used in the import-archive command).

Point being, there's not a lot of places where fixEncoding runs under python3 right now, pretty much just import-archive --type maven.

So I wonder if import-archive works on py3 as-is, or if it would work better if fixEncoding returned str.

btw, the .encode('utf-8') was added here:

commit e565217a73e82ee1f58c7f170bb8bdec76c66e99
Author: Mike McLean <mikem@redhat.com>
Date:   Tue Dec 8 22:40:05 2009 -0500
    avoid unicode errors involving changelogs (rhbz 545387, patch by dmach)

https://bugzilla.redhat.com/show_bug.cgi?id=545387

Interestingly, fixEncoding is barely used client side. It's used several places in the hub, web, and builder, but in the client lib the only place is in parse_pom (which itself is mostly used on hub/builder, but is used in the import-archive command).
Point being, there's not a lot of places where fixEncoding runs under python3 right now, pretty much just import-archive --type maven.
So I wonder if import-archive works on py3 as-is, or if it would work better if fixEncoding returned str.

Tested import-archive --type maven with current master and https://pagure.io/fork/julian8628/koji/c/3d55e9d

They both work fine.

ping @mikem @tkopecek for reviewing this candidate

When rebased, this breaks one of the newer unit tests. I've made an adjustment to that test here:
https://github.com/mikem23/koji-playground/commits/pagure/pr/578

This function works well enough, but I'm cautious about how we will want to approach this long term.

On my branch, I've renamed the function to _fix_print and added a docstring. I want to discourage clients from using this function for now. I think we'll end up revisiting this as we port more of the code to python3.

Also I removed the if not value case. So far, we're only using this is a place where we expect an actual string, so let's not get ahead of ourselves. We don't know what we want to do with other values yet, if anything.

If that looks ok to you, I can merge it tomorrow.

If that looks ok to you, I can merge it tomorrow.

That's good. I've rebased your commit into this PR

rebased onto 3464adae2e8a42f5f1a4249d7478cbb0ffb418a3

1 new commit added

  • fix unit test - test_formatChangelog

Commit 62d4d1d1 fixes this pull-request

Pull-Request has been merged by mikem

Metadata