New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PoC library simplification #6
PoC library simplification #6
Conversation
Here is an aggressive PoC rewrite to simplify the library, while retaining most of the functionality. Some specific points: - Remove the special `MasterStatus` class. This design was awkward, because it was a subclass of Status, but provided functionality that should have been provided by the StatusPool, resulting in a lot of edge case code. - Merge required functionality into the StatusPool. - Remove unnecessary classes; the clobberer classes could be pure functions. - Rename clobberer to summarizer - Leverage more existing code: - use UnknownStatus as-is - use StatusBase.from_name - Remove some magic, but retain just enough to still support `pool.status_name` get/set access shortcuts. - Remove the static subclass based approach to defining the pool (it's cool, but feels too magical, and I think in practice the procedural method of building the pool will be more practical). - Consolidate status tag and attr into a single name. - Remove the externally managed private variables from Status class. - Remove static class variables from Status class (originally for tracking ids for ordering)
The main reason behind the static pool definition with Statuses as descriptors was the code completion/typing it enabled. self.status = StatusPool(self, skip_unknown=True, auto_commit=self.AUTO_COMMIT)
self.status.workload = Status("workload")
self.status.relation_1 = Status("relation_1")
self.status.relation_2 = Status("relation_2") While I see that dynamically adding things works just as well, I'm not convinced it's more ergonomic. A downside of doing it this way is that it clutters I'm going to give it some thought. |
Another thought on the dynamic-adding versus statically defined:
|
I'm not a fan of designing hacky or magic code to optimise for editor code completion. Firstly because this is python; if we wanted real typing we would use a language with real typing, so we just need to grimace and bear it (ie. write nice pythonic code and deal with the lack of perfect typing). Secondly, I've seen this kind of thing before - it seems really nice to have great autocompletion, until you look back at the unmaintainable code that made it possible. Gotta make a trade off here.
How about this - doesn't clutter init: def build_status_pool(charm):
pool = StatusPool(charm, skip_unknown=True, auto_commit=True)
pool.workload = Status("workload")
pool.relation_1 = Status("relation_1")
pool.relation_2 = Status("relation_2")
return pool
class Charm(...):
def __init__(self, ...):
self.status_pool = build_status_pool(self)
Huh I've never seen that before in python. What's going on there? If this is simply so the pool can be dynamically built at top-level code, does a
I say no. There's still the non-syntax sugar method of accessing the status from the pool, and if a dev wants to name a status |
To add more about why this is fine... Also note that there are two options for setting a name here:
In case 1, static string, the dev has full control and can ensure they set it to something that can be used as an attribute name if they want to use that. In case 2, dynamically, you're not going to be using the attribute shorthand anyway. eg.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, I think there needs to be some pondering, discussion, and agreement on how exactly we want the user experience/api to evolve before engaging in major implementation mods. Otherwise we'll just be code thrashing unnecessarily and will just settle on something less than optimal because we got worn out writing/reviewing code.
status_pool = StatusPool(self) | ||
status_pool.add(Status("workload")) # this tracks workload status | ||
status_pool.add(Status("relation_1")) # this tracks my integration #1 | ||
status_pool.add(Status("relation_2")) # this tracks my integration #2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about something like:
status_pool = StatusPool(self, 'workload', 'relation_1', 'relation_2')
status_pool.set_status("relation_1", ActiveStatus('✅')) | ||
status_pool.relation_2 = ActiveStatus('✅') | ||
|
||
workload_status = status_pool.workload | ||
workload_status.status = ActiveStatus('✅') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like trying to accommodate everything and the kitchen sink here is a bit much. I'd prefer we settle on one canonical way to set/retrieve status. I'd almost be inclined to just do status_pool['relation_1'] = ActiveStatus()
self, | ||
charm: CharmBase, | ||
key: str = "status_pool", | ||
summarizer: Callable[[List[Status], bool], str] = summarize_worst_only, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue here would be if the library becomes widely used, we'd get divergence between what gets reported in juju status - since everyone will be tempted to customize it differently. I think I'd prefer we just have one canonical presentation of overall status - unchangeable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I have to agree, although I do like the customisability. I left some thoughts on the internal design doc about this too.
As a general comment though, I do agree that auto-completion should not be the primary driver for implementation design - although it is certainly an important consideration. |
I think at this stage it will benefit from actually using this library in some charms, and iterating on the api based on usage patterns and painpoints that emerge. This early stage of development is really the only opportunity we'll have to make many breaking changes, as nothing is depending on it yet. There's an adage that says one should write client code first. As in, write PoC code in a client (ie. a charm) how it most makes sense in context, then write the library/api code based on how you found you needed to use it. This is useful because it avoids writing an api that you imagine to be useful and ergonomic, but then discovering it's not so much when actually using it in a real world charm. This has kind of been done with the example charm, but it's still difficult to simulate real-world use cases from a small example. For example, with the experimenting I've been doing the last couple of days to integrate into a sunbeam charm, it's become apparent that pretty much all statuses will need to be dynamically added to the pool via add_status, so the static class-based declaration of the pool may not actually be that useful perhaps (I could be wrong, but that's just an example). This is why I'm in favour of drastically reducing the complexity and featureset here, then gradually adding features back in as we find a need (which will include iterating on the api to ensure the new features form a coherent set). |
fyi I'm now using this branch to iterate as I experiment with using it. See related patches:
|
Closing because we're going to avoid a major rewrite here, in favour of smaller iterations and prototyping elsewhere. |
Here is an aggressive PoC rewrite to simplify the library,
while retaining most of the functionality.
Some specific points:
MasterStatus
class. This design was awkward,because it was a subclass of Status, but provided functionality
that should have been provided by the StatusPool,
resulting in a lot of edge case code.
to still support
pool.status_name
get/set access shortcuts.(it's cool, but feels too magical, and I think in practice
the procedural method of building the pool will be more practical).
Fixes #7
Fixes #8
Fixes #9
Fixes #10
Fixes #11
Fixes #12
Fixes #13
Fixes #14
Fixes #15
Fixes #16
Fixes #17
Fixes #19
Fixes #20
Fixes #21
Fixes #22