refactor!: move _base_plan to last position in Container.__init__

[tonyandrewmeyer]

I feel that it's messy to have an argument that's clearly (because of the leading _) intended for internal use only in the middle of the positional arguments. This PR moves it to the end.

#137 is a larger alternative, where many of the dataclass arguments become kw-only.

[PietroPasotti]

Yeah, I agree that the dataclass base is sometimes a bit restrictive when it comes to designing the signatures.
I wouldn't mind reworking that choice.
Would that mean reintroducing something like a _DCBase and do some magic in __init__?

[tonyandrewmeyer]

Yeah, I agree that the dataclass base is sometimes a bit restrictive when it comes to designing the signatures. I wouldn't mind reworking that choice. Would that mean reintroducing something like a _DCBase and do some magic in __init__?

In Python 3.10+, we could do something like this (all comments/docstrings dropped for simplicity):

@dataclasses.dataclass
class Container:
    name: str
    can_connect: bool = False
    _: dataclasses.KW_ONLY
    _base_plan: dict = dataclasses.field(default_factory=dict)
    layers: dict[str, pebble.Layer] = dataclasses.field(default_factory=dict)
    service_status: dict[str, pebble.ServiceStatus] = dataclasses.field(
        default_factory=dict,
    )
    mounts: dict[str, Mount] = dataclasses.field(default_factory=dict)
    exec_mock: _ExecMock = dataclasses.field(default_factory=dict)

I don't love the use of an argument as a sentinel, but that's what Python decided to do.

You then get an __init__ like this:

def __init__(self, name: str, can_connect: bool = False, *, _base_plan: dict = <factory>, layers: dict[str, ops.pebble.Layer] = <factory>, service_status: dict[str, ops.pebble.ServiceStatus] = <factory>, mounts: dict[str, scenario.state.Mount] = <factory>, exec_mock: Dict[Tuple[str, ...], scenario.state.ExecOutput] = <factory>) -> None:

And you can use it like:

# good
Container("foo")
Container("foo", False)
Container("foo", can_connect=False)
Container("foo", layers=...)
Container("foo", exec_mock=..., layers=...)

# bad
Container("foo", False, my_plan, my_layers)

I actually think ops would have benefited from a bit of this as well, but most of the time you're not actually creating objects in ops (it's doing it for you from Juju data) so you don't really notice it.

I think a lot of the state classes have a small number of arguments where it's predictable or obvious enough that positional arguments work (and IDEs help a lot here of course) but the ones that have a lot of args could probably benefit from some being keyword-only.

In older Python (we presumably need to keep supporting 3.8) we can do the same thing, we just have to implement the __init__ ourselves, which is not nice.

[PietroPasotti]

could use attrs, I had some good experiences with it in the past, if you go beyond the cheekiness of attr.ib and attr.s

[benhoyt]

I agree that probably everything but the non-default args should be keyword only. However, as Tony mentions, dataclasses.KW_ONLY is Python 3.10+. Can we get away with requiring 3.10+ for Scenario, even while Ops still requires 3.8? (Maybe we can do that?)

For now, I think it's sensible to just move _base_plan to the end.

[tonyandrewmeyer]

Closing in favour of #137.