Minor improvements

[johnsca]

• Sleep a small amount in block_until to avoid CPU-heavy busy-wait
• Better repr of ModelEntities for debugging
• Better errors when statuses accessed on dead units and applications

[tvansteenburgh]

Forgive my density, can you explain the benefit of this change in more detail?

[johnsca]

np, probably should have commented that. The intention is to have it run though the checks in ModelEntity.__getattr__ so that we get a more useful error if accessed on a dead entity (rather than NoneType is not subscriptable).

[tvansteenburgh]

Wouldn't self.status['current'] do the same thing?

[petevg]

It's a little odd to me to see you call self.__getattr__ rather than getattr(self, ..., but it's not that big of a deal. If the class is always guaranteed to have a status, I can see dropping it, per @tvansteenburgh

[tvansteenburgh]

self.status is the same as self.data['status'] (because of the __getattr__ that we have on ModelEntity. But that won't work for hyphenated names, like 'agent-status'.

My main complaints with this is that 1) it makes my eyes bleed 2) it's hard to understand why it's there unless you already know (difficult to maintain) and 3) we (or someone else) will forget to use this technique in the future.

I'd like to see us come up with a cleaner way to achieve the same result if possible.

[johnsca]

There was a naming conflict with self.status in one of the methods (which also caused an issue with using getattr()), and the others included hyphenation. I'll make something cleaner because I agree that it sucks.

[petevg]

This looks a lot nicer :-) LGTM.

[tvansteenburgh]

This addresses two of my issues nicely. But...

1. Wouldn't it be nicer to only have to call _check_alive() in one place?
2. _check_alive() is not the best name, because ModelEntity already has alive and dead properties, and alive doesn't mean the same thing as _check_alive(). For example, say you have a ModelEntity with self.data == None. So, it's dead. In that case, entity.previous().alive == False (but the entity is actually dead in the real Juju model), but entity.previous()._check_alive() == True (because the entity at it's previous state still has data).

I would prefer to just do the None check in the data property itself, but there are some bits of code that actually want to know if that returns None, and handle it in some way.

I wonder if we should add a safe_data property that returns data, or raises DeadEntityException if it's None.

Sorry, I'm really not trying to be difficult here, I just don't feel like this is quite right yet. Definitely open to more discussion/ideas.

[johnsca]

@tvansteenburgh Updated.

[tvansteenburgh]

s/self_data/safe_data/