Code review comment for lp:~vila/bzr/799212-non-ascii-confs

Revision history for this message
Vincent Ladeuil (vila) wrote :

> This set of changes is what I had in mind.

Good.

>
> As I understand it, bug 502060 is about TransportConfig lacking
> try/except,

Yes.

> then bug 688677 and bug 797246 are the same non-utf8 issue
> in copied code.

Not exactly but close enough.

> However I don't understand where bug 799212 comes in
> or even what it is really from the report.

Bah, that's just lp being helpful because I reused the same branch (see lp:bzr, a previous fix has already landed).

>
> One change I'd suggest is making ConfigContentError take the
> UnicodeError instance as a second param so the UI has the option of
> giving more details like where in the file to look for the bad bytes.

I thought about that even tried to point to the right line number.

But whatever trick you may want to do at this point you have to inspect the content of the file again. So you may as well split its content and try to decode each line (which I thought was a bit too much) so adding the UnicodeError doesn't add much (and I didn't want to dive into the various possible errors).

We may revisit that if we get new bug reports from people not getting the clue about the encoding.

« Back to merge proposal