api : Encoded certain endpoint parameters.

[aryanshridhar]

Rectified parameters in certain api endpoints to deprecate dictionary parameters .

Affected Endpoints :

GET get_profile
POST update_presence
GET get_users
GET get_members
GET get_subscriptions (was named as 'list_subscriptions' until commit).
PATCH mute_topic
POST render_message
POST create_user
PUT update_storage
GET get_storage
POST set_typing_status

As discussed within zulip/zulip#16698

[andersk]

Deprecating is not the same as removing—we still want to support the old syntax with a warning for some deprecation period.

Also, this is not the only function affected by this issue.

[aryanshridhar]

Hey @andersk , upon grep -rnw . -e 'list_subscriptions' , I see the following functions calling the list_subscriptions function :

/integrations/jabber/jabber_mirror_backend.py:278:        stream_infos = get_stream_infos("subscriptions", zulipToJabber.client.list_subscriptions)
./integrations/bridge_with_irc/irc_mirror_backend.py:39:        resp = self.zulip_client.list_subscriptions()
./build/lib/integrations/jabber/jabber_mirror_backend.py:278:        stream_infos = get_stream_infos("subscriptions", zulipToJabber.client.list_subscriptions)
./build/lib/integrations/bridge_with_irc/irc_mirror_backend.py:39:        resp = self.zulip_client.list_subscriptions()
./build/lib/zulip/examples/list-subscriptions:21:print(client.list_subscriptions())
./build/lib/zulip/__init__.py:1281:    def list_subscriptions(self, request: Optional[Dict[str, Any]] = None, **kwargs: Any) -> Dict[str, Any]:
./zulip/examples/list-subscriptions:21:print(client.list_subscriptions())
./zulip/__init__.py:1281:    def list_subscriptions(self, request: Optional[Dict[str, Any]] = None, **kwargs: Any) -> Dict[str, Any]:

But none of them have a parameter of {'include_subscribers' : bool_value} . Could you guide me on mentioning the functions that need to be altered due to this issue .

[andersk]

These all share the same antipattern:

    def get_profile(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def update_presence(self, request: Dict[str, Any]) -> Dict[str, Any]:
    def get_users(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def get_members(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def list_subscriptions(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def mute_topic(self, request: Dict[str, Any]) -> Dict[str, Any]:
    def render_message(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def create_user(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def update_storage(self, request: Dict[str, Any]) -> Dict[str, Any]:
    def get_storage(self, request: Optional[Dict[str, Any]] = None) -> Dict[str, Any]:
    def set_typing_status(self, request: Dict[str, Any]) -> Dict[str, Any]:

[aryanshridhar]

Is there any changes to be made @andersk ? Or should I go ahead to fix failing mypy failures .

[neiljp]

@andersk Would a decorator to handle the migration work here, do you think?

[andersk]

A decorator might have the disadvantage of making the pydoc confusing and the mypy types poor (all mypy can guarantee about a decorator is basically “congratulations, you have a function!”).

A helper function pattern like this would be fine:

def deprecated_request(request: Optional[Dict[str, Any]]) -> Dict[str, Any]:
    if request is not None:
        logger.warning(…)
        return request
    return {}

def list_subscriptions(self, request: Optional[Dict[str, Any]] = None, **kwargs: Any) -> Dict[str, Any]:
    return self.call_endpoint(…, request={**deprecated_request(request), **kwargs})

[neiljp]

@andersk I agree that this approach is simple - and almost suggested something similar, ie. putting all of the deprecation-migration code together.

Re decorators, I did think mypy could handle them - since we have them annotated in the main repo - though I suppose it's one thing to annotate and another to know what is actually validated via them being present. I'd also have thought @wraps would handle docstrings, and would therefore take it that docstring-consumers like pydoc would be OK too.

[aryanshridhar]

Hey , @andersk @neiljp , Could you review it again ?

[timabbott]

I think this change makes sense to me -- @aryanshridhar can you rebase this?

I'm a bit reluctant to merge this while we're unable to update python-zulip-api in zulip/zulip, which we'll definitely need to do in order to update documentation for the modified endpoints.

(Or did we resolve that issue and I just didn't notice?)

[aryanshridhar]

Rebased it @timabbott, As far as I remember, there was no further work done related to this pr in zulip/zulip. Opened zulip/zulip#17804 to update the docs (Converted it to draft until this pr looks fine).

[timabbott]

I'm going to hold off on this until after https://github.com/zulip/python-zulip-api/pull/692; let's chat about it next week.

[timabbott]

@aryanshridhar can you rebase this?

[zulipbot]

Heads up @aryanshridhar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.