feat(snap): add snap interfaces module#570
Conversation
tonyandrewmeyer
left a comment
There was a problem hiding this comment.
The code seems fine, but I'm not convinced about the API here, sorry. Am I forgetting this being discussed previously?
|
|
||
|
|
||
| def connect( | ||
| plug_snap: str, plug: str, slot_snap: str | None = None, slot: str | None = None, / |
There was a problem hiding this comment.
Why positional only? I think I would prefer keywords here, because it's awkwardly a pair of strs and then a one-or-two str 'pair'. Plug->slot works ok for people who normally read left-to-right, I think, but then you're relying on CLI familiarity or seeing the signature to know the order of the rest.
I feel like I would have gone with
| plug_snap: str, plug: str, slot_snap: str | None = None, slot: str | None = None, / | |
| plug_snap: str, slot_snap: str | None = None, *, plug: str, slot: str | None = None, |
Although with no default for plug, that is a little odd.
I think I'm ok with no kwonly, but I don't like positional only unless there's a motivation I'm missing.
There was a problem hiding this comment.
Connect and disconnect both have this basic problem. The CLI uses colon-separated strings for the snap and its plug/slot, like:
snap connect [connect-OPTIONS] <snap>:<plug> [<snap>:<slot>]
snap disconnect [disconnect-OPTIONS] <snap>:<plug> [<snap>:<slot>]
Translating this to Python, I thought that separate positional arguments made the most sense. But there are other options. We've resisted creating a reified Snap type elsewhere, so we should probably see how far we can get with that here (so no Snap('foo').plug('bar').connect(Snap('baz').slot('bartholemew')).
Sticking with the flat, functional design, a few ideas spring to mind:
- Four separate positional arguments (current implementation).
- Two colon-delimited-string positional arguments.
- Two positional arguments that can either be a string or a
tuple[str, str].
I do suspect that reducing to two arguments would be a net win.
For connect, the slot snap can be omitted entirely, or provided as a bare snap name (no slot specified).
disconnect supports a single snap form, but with different semantics: if a single colon-separated snap name is provided, whether it's a slot or plug is implicit. Actually, I'm now wondering if my implementation even gets this right -- in the single snap disconnect case, I treat it as the slot snap. I'll have to look into this further.
As to the arguments being positional-only, that's why -- hard to have a name that should be used as a keyword argument if the meaning of the argument changes based on the number of arguments.
There was a problem hiding this comment.
We've resisted creating a reified
Snaptype elsewhere, so we should probably see how far we can get with that here
Yeah, I'm onboard with that, although I do feel like it would have avoided things I don't like in other PRs too.
- Two colon-delimited-string positional arguments.
I see why you didn't go with this, and it does feel less Pythonic and more just exposing the snap CLI in a raw way. I do think it's interesting that the API doesn't work this way.
- Two positional arguments that can either be a string or a
tuple[str, str].
I think this is worth considering.
connect('somesnap')(is this possible?)connect('vlc', 'audio-record')connect(('jhack', 'ssh-read'), 'snapd')(maybe a bad example since I think snapd is the default, but it's the one I know)connect('foo', ('bar', 'face'))connect(('foo', 'one'), ('bar', 'two'))(I don't love this, but probably it is most rare?)
As to the arguments being positional-only, that's why -- hard to have a name that should be used as a keyword argument if the meaning of the argument changes based on the number of arguments.
Yeah, which is, I would argue, evidence that it's not the right API.
There was a problem hiding this comment.
We've resisted creating a reified
Snaptype elsewhere, so we should probably see how far we can get with that hereYeah, I'm onboard with that, although I do feel like it would have avoided things I don't like in other PRs too.
I wouldn't be opposed to revisiting that decision. Maybe it's too late to do it before release, but we could always build a Snap class on top of the lower-level functional API later.
- Two colon-delimited-string positional arguments.
I see why you didn't go with this, and it does feel less Pythonic and more just exposing the snap CLI in a raw way. I do think it's interesting that the API doesn't work this way.
Yeah the fact that the API explicitly accepts named 'plugs' and 'slots' arguments is a point in favour of handling that differently here.
- Two positional arguments that can either be a string or a
tuple[str, str].I think this is worth considering.
connect('somesnap')(is this possible?)connect('vlc', 'audio-record')connect(('jhack', 'ssh-read'), 'snapd')(maybe a bad example since I think snapd is the default, but it's the one I know)connect('foo', ('bar', 'face'))connect(('foo', 'one'), ('bar', 'two'))(I don't love this, but probably it is most rare?)
I'll explore this option further, I kind of like it too. With the two argument form, we could use plug and slot as the names too, and keyword arguments start to look nice again.
As to the arguments being positional-only, that's why -- hard to have a name that should be used as a keyword argument if the meaning of the argument changes based on the number of arguments.
Yeah, which is, I would argue, evidence that it's not the right API.
It's not very common, though we do see this in stdlib constructs most developers would be familiar with, like range and slice. But I'm hopeful that the two argument approach solves things.
One thing I'm not sure about is how to spell things like [dis]connect(plug=None, slot=(mysnap, myslot)). Maybe like that?
| name to disconnect (two-argument form). | ||
| slot_snap: The snap providing the slot. Omit for the two-argument form. | ||
| slot: The slot on ``slot_snap``. May be omitted to match any slot on ``slot_snap``. | ||
| forget: If ``True``, also forget any manual connection preference, so the interface |
There was a problem hiding this comment.
I'm not super familiar with snap interfaces. I assume people who are would understand this, but maybe we should help others out more? Is there a way to use the library to set a manual connection preference? Where would I go to learn more about what this means, as a library user?
Also, I think we would generally use true (meaning truthy) here, not True.
| ) -> None: | ||
| """Disconnect a plug from a slot. | ||
|
|
||
| May be called in two forms: |
There was a problem hiding this comment.
I really don't like this as a Python API style. Did this get discussed and resolved already when everything was in one PR?
kwonly would solve this, or two methods would solve this. I definitely need convincing that positional args can be entirely different objects based on the number provided.
This PR adds the snap library functionality using the
/v2/interfacesendpoint, mirroring the snap CLI commandssnap connectandsnap disconnect.