Papercuts

[AdamIsrael]

Fixes a few papercuts:

config_changed hook
fix status_set after successful config_changed
consistent use of status_set

[chuckbutler]

nice catch here

[marcoceppi]

Not a nice catch. There should be a finally and else. Else means no exception raised. Finally means everything after including exception. With this change vpe.configured will be set always

[johnsca]

It would probably be less confusing to just move the else: code inside the try block at the bottom.

Edit: The main reason to use the else clause with try is if you expect the code in the else clause to raise an exception that would be caught by the except which you don't want to catch. In this case, your except clause is too broad and you should catch something more specific than Exception (both of those could reasonably be ValueErrors, or, even better, you could create a ValidationError subclass), at which point it would be reasonable to move the code from the else block into the try.

[chuckbutler]

#TIL - python has except/else - https://docs.python.org/3/tutorial/errors.html

[marcoceppi]

It's a good point, moving the set_state inside of the try block is probably best

[marcoceppi]

What is this? We never set a blocked state?

[AdamIsrael]

Line 37 set the state. I found setting the config values never cleared the state.

[johnsca]

Should line 37 be status_set instead of set_state?

[AdamIsrael]

Good catch, I think you're right.

[AdamIsrael]

Fixed the else and state/status switch. I redeployed and confirmed that it's working as expected now.

[chuckbutler]

I'm +1 on this PR

As my initial glance was interrupted and i got completely schooled on some python :) I'll reserve the actual merging to @marcoceppi

[marcoceppi]

Thanks for cleaning up my typos with set_state and status_set. My only real comment is I really prefer all lower case status messages since all of the tabular format is lowercase. It's not enough to block this but if someone feels like going through and updating this...

[marcoceppi]

This syntax is odd to me. maybe if ['pass', 'vpe-router', 'user'] not in cfg.keys() instead

[AdamIsrael]

I tried that but it didn't work for me.

[marcoceppi]

yeah, I see that. I'd prefer this instead:

>>> cfg
{'vpe-router': None, 'obama': None, 'user': None, 'pass': None}
>>> all(k in cfg for k in ['pass', 'vpe-router', 'user'])
True
>>> del cfg['pass']
>>> cfg
{'vpe-router': None, 'obama': None, 'user': None}
>>> all(k in cfg for k in ['pass', 'vpe-router', 'user'])
False

so

if all(k in cfg for k in ['pass', 'vpe-router', 'user']):

[marcoceppi]

⛵️