search

huin / goupnp

UPnP client library for Go (#golang)
BSD 2-Clause "Simplified" License
417 stars 84 forks source link

Contexts on HTTP calls #42

Closed JulianKnodt closed 2 years ago

JulianKnodt commented 3 years ago

I was wondering whether it would be possible to add contexts to any client calls, to make it more idiomatic. It would be easy to implement as a breaking change, as you could just stick a context into the codegen template, but that's definitely painful to old users of the package. I think it also may be a lot of bloat to add a new set of methods for context, but I was wondering what you thought of it.

huin commented 3 years ago

Short answer: yes, this library really should use contexts, and not just in the SOAP calls.

This is something that I started thinking about doing a while back, and there was some work I did on the v2 branch to do that and other things. For one reason or another, I never completed that work, and since then various fixes and changes have merged into master.

There's a lot of things I'd do differently in this package if I were to redesign them, but they'd almost all be breaking changes. It would likely be best to do such breaking changes in bulk on a separate v2 package to avoid breaking existing users.

If this package branched out to a v2, it'd be good to know which things need doing, not just contexts. IPv6 is one thing, although I know basically zero about how multicast/UPnP works with IPv6. There's also other breaking API changes I'd likely make, which would probably make substantial differences to how the SOAP APIs are defined.

I need to ponder that a bit, but would be interested in your thoughts and others on that.

JulianKnodt commented 3 years ago

Hm, I'm not particularly experienced in releasing packages and maintaining compatibility for older versions. What I think would be best would probably be having a numbered version as you suggest, and seems to be recommended by the golang team. One thing for a v2 might be to release a non major version, and expect breaking changes such as adding IPv6 or others before reaching a completed v2. I suspect that trying to do everything in bulk would probably take way too long and lead to the package not being completed, as is the case when there is feature creep.

One other approach patching existing clients might be to make it so that a UPnP client could patch in a context for the next call... but that seems really hacky and not ideal. But that's just a random idea.

huin commented 2 years ago

So hopefully commit 5a0d4bd fixes your current needs. I've made some start on a v2, but as you note it will likely take a while for the limited free time I spend on goupnp. Let me know if this change doesn't work well for you.

JulianKnodt commented 2 years ago

I think the actions still need to use the context inside of the http call by attaching it to the request, but it's good as an entry point!

huin commented 2 years ago

I think the actions still need to use the context inside of the http call by attaching it to the request, but it's good as an entry point!

Ah, that should be in 6145404, which I submitted prior.

JulianKnodt commented 2 years ago

ahhhh cool cool then it should be good from my pov!