django-oscar / django-oscar-docdata

Docdata Payments Gateway integration for django-oscar
Apache License 2.0
23 stars 11 forks source link

Raise error if order.status is unknown #42

Closed jedie closed 5 years ago

maerteijn commented 5 years ago

I'm not really keen on the RuntimeError exception here, nor that a unknown status can occur.

jedie commented 5 years ago

Yeah, that's never supposed to happen. But maybe they are thinking about introducing a new status however. That's the general way I do it. I think it's better to count on the unexpected ;)

maerteijn commented 5 years ago

I disagree on the part to write code to count on the unexpected. When an unexpected result comes it should break.

And this sandbox application is, as I stated before, not meant to be a full blown working application but a starting point for your own project so there is no need to add a lot of safety nets here.

vdboor commented 5 years ago

Perhaps logger.warning(...) instead? Or raise NotImplementedError(...)?

maerteijn commented 5 years ago

logger.warning would make sense, so if you add that I'll merge it

jedie commented 5 years ago

I don't think a warning would be enough. That might quickly be overlooked, as you sayed:

When an unexpected result comes it should break.

...and a warning will not break.

And this sandbox application is [...] a starting point for your own project so there is no need to add a lot of safety nets here.

IMHO the starting point should be a good template ;)

maerteijn commented 5 years ago

...and a warning will not break.

You are right. Can you change it to raise the DocdataStatusException ?

jedie commented 5 years ago

done