Repository navigation
Conversation
Add property_value() for canonical and custom property names, and has_property() for name-only checks or exact CFProperty matches. Cover enum and string names, missing properties, empty and None values, exact matches, and first-match lookup semantics.
Keep existing merge semantics and add direct coverage for replacement, preservation of unmanaged properties, omission of managed properties, and non-mutation.
Migrate factories, fixtures, queries, and property payloads to PyCFClientAdapter and typed CFChannel/CFProperty objects. Keep property inspection and assertion behaviour explicit for separate follow-up commits.
Replace the free merge helper with non-mutating CFChannel.merged_properties(). Preserve name-based replacement, managed-property omission, ordering, and alias construction without changing parent channels.
Add CFChannel.property() for canonical and custom names. Reuse it for value and name-existence lookups while preserving exact-property matching.
…ion tests Replace property-list inspection with CFChannel.property(), property_value(), and has_property(). Keep assertion-strengthening changes separate.
Add IOCInfo.channel_properties() and status_properties() and use them for channel creation, active updates, orphaning, and aliases. Preserve reassignment semantics with a non-mutating IOC copy: previous identity, current owner and timestamp. Cover property ordering, fresh objects, and unchanged source IOC state.
Compare channels by name rather than response order and reject empty-result property checks. Write the manual property before testing its preservation, assert removal outcomes, and check all relevant channels. Add unit coverage for typed integration helpers and the adapter factory boundary.
|
anderslindho
left a comment
anderslindho
left a comment
There was a problem hiding this comment.
Some further cleanup/improvements wanted, but overall nice and clean work (esp commit splits) 👍
| return prop in self.properties | ||
| return self.property(prop) is not None | ||
|
|
||
| def property(self, name: Union[CFPropertyName, str]) -> Optional[CFProperty]: |
There was a problem hiding this comment.
This API feels messy. There are three overlapping methods:
property, which hides thepropertybuilt-in inside the class body and, confusingly, isn't a Python@propertyproperty_value, which isproperty(name).valuewith a None checkhas_property, which isproperty(name) is not None, except when it's given aCFProperty. Then it switches to an exact match on name, owner and value.
So two of them fetch a property (or its value), and the third only checks whether it exists, at about the same cost. As far as I can tell, none of the three are used by the processor. has_property and property_value are only called from the tests, and property is only called by those two.
I'd rather you take this further and cut it down to one lookup method, e.g. get_property(name) -> Optional[CFProperty]. A test that needs to check the property exists can then assert that the result is not None. For an exact match, prop in channel.properties already works and needs no helper.
| owner: str | ||
| properties: List[CFProperty] | ||
|
|
||
| def merged_properties( |
There was a problem hiding this comment.
I'm also not sure merged_properties belongs on CFChannel. As far as I can tell it isn't really a merge. It's one-way: the new properties overwrite the channel's existing ones, and an existing property whose name is in managed_properties is dropped unless new_properties supplies it. That's (maybe) fine as behaviour, but the method doesn't fit the model well:
managed_propertiesis processor policy (the property names this recceiver is the authority for), not channel data. As a method, the model now takes processor config as an argument.- The channel isn't always the one being updated. In the alias branch,
aprops = cf_channel.merged_properties(...)uses the parent channel's properties as the base for a different (alias) channel. That's also the only reason this has to return a new list rather than update the channel. - The orphan paths call it without
managed_properties, so every existing property is kept. I think that's intentional (an inactive channel keeps its last known properties), but it's a behaviour switch hidden behind an optional argument.
I'd rather keep this in the processor as a plain function over property lists, with a name that says what it does, e.g.:
def _apply_update(existing: List[CFProperty], update: List[CFProperty], owned: Set[str]) -> List[CFProperty]:
"""Overlay `update` onto `existing`, dropping existing props whose name is in `update` or `owned`."""The alias case then just passes the parent's list. I know this is mostly moved code, so I don't consider it blocking. But if the goal is a cleaner model, I'd leave this one out of it.
Alternatively we just drop and inline it.
| if old_property.name not in new_property_names and old_property.name not in managed: | ||
| new_properties = new_properties + [old_property] | ||
| return new_properties | ||
| return replace(last_ioc_info, owner=ioc_info.owner, time=ioc_info.time).channel_properties(recceiverid) |
There was a problem hiding this comment.
Nit: replace(last_ioc_info, owner=..., time=...) builds an IOCInfo that doesn't correspond to any real IOC, just to reuse channel_properties. The docstring explains it, but letting channel_properties take optional owner/time overrides would say the same thing without the stand-in object. Not blocking.
2 participants
Reduces the complexity of the some of the processor code.