GeezerVison

What is the trigger for this change? Do I actually have a test that uses a non-ascii charset name? If I did it should be an error case, since non-ascii is not permitted in charset names per the RFCs. I'm surprised I don't appear to be registering a defect for that, though I didn't go through the code enough to be sure I don't ;)

Regardless it isn't clear to me that 'sanitizing' is a useful operation. It isn't likely to produce a valid charset name, we should just be falling back to ascii at that point. What led you to choose this approach?

This is currently done by normalize_encoding.

OK. emal doesn't call lookup directly and no tests fail without the changes.

I presume you did this to preserve backward compatibility. Unless I'm missing something, I don't think we should bother to do that. Given a non-ascii charset name, there are two possible outcomes from the current code: the name after sanitizing is not a valid codec name, or it is. If it is valid after sanitizing, there are two cases: the sanitized name results in successful decoding, or it does not. It is only the first of these second two cases that would be affected by the post-deprecation change.

How often would that case occur in reality? I would guess it would be a vanishingly small number of cases, if it ever occurs at all.

I think it will be better to remove the changes to the email package from this PR. If anyone sees the deprecation warning maybe they'll open an issue, but I'm betting nobody ever sees it from the email package. The behavior after the deprecation is over is the behavior we want: if the codec name contains non-ascii it is not a valid codec name, so any non-ascii in the text being decoded using that charset name will ultimately get turned into the 'unknown character' glyph when decoded by the email package.

I presume you did this to preserve backward compatibility.

Yes, I'm no email expert and I did not dig into the specifications, so I did this to not change any behaviour. I can remove it.

What't the conclusion here ? I still see the email package changes in place, but they look pretty harmless to me.

@bitdancer It was indeed tested.

Ah, I missed that. I guess maybe warnings weren't enabled the way I ran the tests, though I thought I was doing so...

What do you want to do, keep it like so or revert to my backwards-compatible approach?

I believe I've confirmed that the tests will still pass after lookup starts raising a ValueError, which is what I was hoping would happen. I wonder if we should actually assert the deprecation warning here, though, so that we are reminded to remove the check when the deprecation turns into reality. I'm fine with whatever is standard practice for such cases, though.

Converted to asserts.