Noissue - [CMS] Avoid overzealous catching of KeyError during conversion

Noissue - [CMS] Avoid overzealous catching of KeyError during conversion

[kzar, 2015-09-21 09:21:48]

Patch Set 1

This is something that kept catching me out (ho ho), I kept having completely
unrelated KeyErrors being caught here during development, which was sometimes
quite confusing.

[Wladimir Palant, 2015-09-21 10:54:29]

https://codereview.adblockplus.org/29328208/diff/29328209/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29328208/diff/29328209/cms/utils.py#newcode54
cms/utils.py:54: raise Exception("Page %s uses unknown format %s" % (page,
format))
I think Sebastian will object to that approach :)

I guess the proper approach would be moving the call outside the try..catch
block.

[kzar, 2015-09-21 11:06:22]

Patch Set 2 : Go back to using try... catch

https://codereview.adblockplus.org/29328208/diff/29328209/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29328208/diff/29328209/cms/utils.py#newcode54
cms/utils.py:54: raise Exception("Page %s uses unknown format %s" % (page,
format))
On 2015/09/21 10:54:29, Wladimir Palant wrote:
> I think Sebastian will object to that approach :)
> 
> I guess the proper approach would be moving the call outside the try..catch
> block.

Yea, I guessed it might have been unpopular. It seemed easier that way but
nevermind. Done.

[Wladimir Palant, 2015-09-21 11:18:02]

https://codereview.adblockplus.org/29328208/diff/29328446/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29328208/diff/29328446/cms/utils.py#newcode52
cms/utils.py:52: converter = converters[format]
I think this variable is better called converter_class - it's not the converter
itself at this point.

[kzar, 2015-09-21 11:23:15]

Patch Set 3 : Gave converter_class variable a better name

https://codereview.adblockplus.org/29328208/diff/29328446/cms/utils.py
File cms/utils.py (right):

https://codereview.adblockplus.org/29328208/diff/29328446/cms/utils.py#newcode52
cms/utils.py:52: converter = converters[format]
On 2015/09/21 11:18:02, Wladimir Palant wrote:
> I think this variable is better called converter_class - it's not the
converter
> itself at this point.

Done.

[Wladimir Palant, 2015-09-21 11:35:20]

LGTM

[Sebastian Noack, 2015-09-21 11:49:29]

LGTM