zulip / python-zulip-api

Python library for the Zulip API.
https://zulip.com/api/
Apache License 2.0
355 stars 358 forks source link

Use `API_FEATURE_LEVEL` to call enpoints ensuring compatibility #710

Closed PIG208 closed 2 years ago

PIG208 commented 3 years ago

683

I experimentally implement a decorator for this purpose.

With this PR, we can sort of "overload" the functions calling endpoints regarding ZULIP_FEATURE_LEVEL. However, I'm not pretty sure about what role client_capabilities would play here.

PIG208 commented 3 years ago

One possible limitation of this approach to be noted: the decorated function should have a function signature compatible with any other alternatives.

eeshangarg commented 3 years ago

I haven't worked with the API bindings in a long time, so there is a lot of context I may be missing here. Having said that, I am really not sure if a decorator is necessarily the right idiom to use here due to what you said about function signatures having to be the same as the alternatives.

The freedom to be flexible and less stringent with function signatures seems like a good goal to have here, so I wonder if there is a simpler implementation approach here. Perhaps a dispatcher class of some sort that every call goes through before settling on which endpoint to call?

timabbott commented 3 years ago

@PIG208 thanks for working on this! I think the decorator strategy is a good one. Ideally, what we want to be doing is presenting the same/modern API in the Python bindings regardless of server version (where possible), and having the @compatible functions take care of rewriting the requests/responses to match the format used by the modern version. For that reason, I don't think the requirement that the functions have a similar signature is likely to be problematic.

timabbott commented 3 years ago

It might make sense to pull the "Zulip CI" piece to a separate PR that we can iterate on independently, if it isn't already.

PIG208 commented 3 years ago

Thank you for the feedback! I will get back to this when I get time.

andersk commented 3 years ago

As discussed in #api design, I think we should use simple if statements instead of complicating this with decorators.

timabbott commented 2 years ago

OK, we can rebase this now.

timabbott commented 2 years ago

Which commits here remain now that #711 is merged?

PIG208 commented 2 years ago

@timabbott 5155ea8. It's a commit that prepare the API for a future change that we are going to make.

timabbott commented 2 years ago

OK; we can probably just go ahead and make that API change in the server and allocate the final feature level, now that we know how we're going to do it. @ligmitz FYI -- did you have a PR for https://github.com/zulip/zulip/issues/18409, or want to make one?

ligmitz commented 2 years ago

I don't have a pull request yet. Can you brief me a little about the steps we are going to take so that I can quickly work a PR ?

timabbott commented 2 years ago

I think we just need to submit a PR to the server that updates the. The merging sequence will be:

timabbott commented 2 years ago

OK, we've completed all but the last step; @eeshangarg can you prepare a python-zulip-api release? Thanks!