Fix #1018 Add optional arguments in WebClient methods by seratch · Pull Request #1099 · slackapi/python-slack-sdk

seratch · 2021-08-19T12:42:51Z

Summary

This pull request fixes #1018 by adding all the optional arguments in WebClient, AsyncWebClient, and LegacyWebClient.

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.
I've run python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.sh after making the changes.

seratch · 2021-08-19T12:43:53Z

slack_sdk/web/async_slack_response.py

        ):
            return self
-        msg = "The request to the Slack API failed."
+        msg = f"The request to the Slack API failed. (url: {self.api_url})"


While running tests, I found that this error message does not have sufficient information.

seratch · 2021-08-19T12:44:58Z

slack_sdk/web/client.py

@@ -77,12 +81,7 @@ def admin_analytics_getFile(
        **kwargs,
    ) -> SlackResponse:
        """Retrieve analytics data for a given date, presented as a compressed JSON file
-
-        Args:


Now that we have all arguments, maintaining Args comment for 240+ API methods becomes more challenging. Instead, we can have a link to the API method document.

I think this will also reduce risk of innacuracy or drift from updates made in one source vs. in the sdk.

seratch · 2021-08-19T12:45:53Z

slack_sdk/web/client.py

+                "team_id": team_id,
+            }
+        )
+        return self.api_call("admin.apps.approve", params=kwargs)


As some endpoints do not support application/json request body, it's safer to use the form body for most APIs.

Interesting, but all endpoints support form POSTs?

@filmaj Yes, they do but interestingly some endpoints do not support application/json.

seratch · 2021-08-19T12:46:28Z

slack_sdk/web/client.py

@@ -119,32 +114,114 @@ def admin_apps_approve(
                "The app_id or request_id argument must be specified."
            )

-        return self.api_call("admin.apps.approve", json=kwargs)
+        kwargs.update(


when we use params (not json), None values will be removed in the api_call method (both async and sync)

seratch · 2021-08-19T12:50:22Z

slack_sdk/web/client.py

        kwargs.update({"channel": channel})
+        kwargs = _filter_none_values(kwargs)


For json=kwargs API calls, having None value in dict can result in null value in JSON data. The Slack API server-side often does not work with the data. Thus, we remove None values here. For params / data, we don't need to worry about it.

codecov · 2021-08-19T13:19:59Z

Codecov Report

Merging #1099 (4541e44) into main (d679144) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
+ Coverage   86.20%   86.27%   +0.06%     
==========================================
  Files         110      110              
  Lines        9955    10413     +458     
==========================================
+ Hits         8582     8984     +402     
- Misses       1373     1429      +56

Impacted Files	Coverage Δ
slack_sdk/web/async_client.py	`92.06% <ø> (-0.74%)`	⬇️
slack_sdk/web/client.py	`93.41% <ø> (-1.00%)`	⬇️
slack_sdk/web/legacy_client.py	`93.11% <ø> (-0.94%)`	⬇️
slack_sdk/web/async_slack_response.py	`90.47% <100.00%> (ø)`
slack_sdk/web/internal_utils.py	`94.26% <100.00%> (+0.09%)`	⬆️
slack_sdk/web/slack_response.py	`93.54% <100.00%> (ø)`
slack_sdk/socket_mode/builtin/client.py	`79.19% <0.00%> (-0.68%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d679144...4541e44. Read the comment docs.

stasfilin · 2021-08-24T13:52:24Z

Wow, nice work,
Waiting when it will be release

srajiang

These look really great! Since there are quite a few changes, it will be good to have a few more eyes pass over this for little things missed.

srajiang · 2021-08-24T16:22:23Z

slack_sdk/web/client.py

@@ -77,12 +81,7 @@ def admin_analytics_getFile(
        **kwargs,
    ) -> SlackResponse:
        """Retrieve analytics data for a given date, presented as a compressed JSON file
-
-        Args:


I think this will also reduce risk of innacuracy or drift from updates made in one source vs. in the sdk.

srajiang · 2021-08-24T16:36:37Z

slack_sdk/web/client.py

@@ -158,7 +250,9 @@ def admin_apps_uninstall(
        team_ids: Optional[Union[str, Sequence[str]]] = None,
        **kwargs,
    ) -> SlackResponse:
-        """Uninstall an app from one or many workspaces, or an entire enterprise organization."""
+        """Uninstall an app from one or many workspaces, or an entire enterprise organization.


We could note here as we do above for admin.apps.restrict requirements, that with an org-level token, enterprise_id or team_ids is required.

@srajiang Thanks. Updated.

srajiang · 2021-08-24T16:58:43Z

slack_sdk/web/client.py

+        org_channel: Optional[bool] = None,
+        target_team_ids: Optional[Union[str, Sequence[str]]] = None,
+        team_id: Optional[str] = None,
+        **kwargs,
    ) -> SlackResponse:
        """Set the workspaces in an Enterprise grid org that connect to a channel.


Can modify to "that connect to a public or private channel"

Good catch. Perhaps, the description was updated afterwards.

srajiang · 2021-08-24T17:52:27Z

slack_sdk/web/client.py

+        self,
+        *,
+        team_id: str,
+        discoverability: str,  # open, invite_only, closed, or unlisted


Perhaps it would be better to remove this additional comment, in case in future the API supports other discoverability options and there is a difference between the SDK and the public docs.

I agree wit this!

srajiang · 2021-08-24T17:55:35Z

slack_sdk/web/client.py

        """
        kwargs.update({"team_id": team_id, "name": name})
-        return self.api_call("admin.teams.settings.setName", json=kwargs)
+        return self.api_call("admin.teams.settings.setName", params=kwargs)

    def admin_usergroups_addChannels(


For some reason the git UI is preventing me from inserting a comment right at line 1153.

Should we make this an Optional field?

team_id: Optional[str] = None,

you're right. Updated.

srajiang · 2021-08-24T18:46:19Z

slack_sdk/web/client.py

+        https://api.slack.com/methods/files.remote.remove
+        """
+        kwargs.update({"external_id": external_id, "file": file})
+        return self.api_call("files.remote.remove", http_verb="POST", params=kwargs)

    def files_remote_share(


Could check for either file or external_id field here or raise a SlackRequestError

srajiang · 2021-08-24T18:49:28Z

slack_sdk/web/client.py

-    def groups_archive(self, *, channel: str, **kwargs) -> SlackResponse:
-        """Archives a private channel.
+    # --------------------------
+    # Deprecated: groups.*


Same comment as with channels.* comment on deprecation. Could point users to conversations.* mapped API methods.

srajiang · 2021-08-24T18:51:27Z

slack_sdk/web/client.py

-        Args:
-            channel (str): Direct message channel to close. e.g. 'D1234567890'
-        """
+    def im_close(


This method is no longer publicly listed on our API docs, even as a deprecated method. Should we remove support for this?

We can remove it in the near future but I didn't do so this time

srajiang · 2021-08-24T18:53:15Z

slack_sdk/web/client.py

-        Args:
-            channel (str): Multiparty Direct message channel to close. e.g. 'G1234567890'
-        """
+    def mpim_close(


Same comment as im.close

srajiang · 2021-08-24T19:02:28Z

slack_sdk/web/client.py


-    def reminders_add(self, *, text: str, time: str, **kwargs) -> SlackResponse:
+    def reminders_add(


There is an unusual options field for recurrence which is not as well documented. Though from investigating it looks like it would accept a string. Did you omit on purpose for now?

Thanks. I just missed the one. Added with optional str type

filmaj

LGTM! I appreciate the links to the methods in the function header.

seratch

@srajiang @filmaj Thanks for helpful comments! I just fixed all of the issues detected here

seratch · 2021-08-24T21:01:59Z

slack_sdk/web/client.py

+                "team_id": team_id,
+            }
+        )
+        return self.api_call("admin.apps.approve", params=kwargs)


@filmaj Yes, they do but interestingly some endpoints do not support application/json.

seratch · 2021-08-24T21:06:27Z

slack_sdk/web/client.py

@@ -158,7 +250,9 @@ def admin_apps_uninstall(
        team_ids: Optional[Union[str, Sequence[str]]] = None,
        **kwargs,
    ) -> SlackResponse:
-        """Uninstall an app from one or many workspaces, or an entire enterprise organization."""
+        """Uninstall an app from one or many workspaces, or an entire enterprise organization.


@srajiang Thanks. Updated.

seratch · 2021-08-24T21:07:56Z

slack_sdk/web/client.py

+        org_channel: Optional[bool] = None,
+        target_team_ids: Optional[Union[str, Sequence[str]]] = None,
+        team_id: Optional[str] = None,
+        **kwargs,
    ) -> SlackResponse:
        """Set the workspaces in an Enterprise grid org that connect to a channel.


Good catch. Perhaps, the description was updated afterwards.

seratch · 2021-08-24T21:08:30Z

slack_sdk/web/client.py

+        self,
+        *,
+        team_id: str,
+        discoverability: str,  # open, invite_only, closed, or unlisted


I agree wit this!

seratch · 2021-08-24T21:10:41Z

slack_sdk/web/client.py

        """
        kwargs.update({"team_id": team_id, "name": name})
-        return self.api_call("admin.teams.settings.setName", json=kwargs)
+        return self.api_call("admin.teams.settings.setName", params=kwargs)

    def admin_usergroups_addChannels(


you're right. Updated.

seratch · 2021-08-24T21:17:28Z

slack_sdk/web/client.py

+        ts: str,
+        cursor: Optional[str] = None,
+        inclusive: Optional[bool] = None,
+        latest: Optional[str] = None,


@srajiang we should not recommend using float values for ts, in general. See slackapi/node-slack-sdk#780 (comment)

seratch · 2021-08-24T21:21:53Z

slack_sdk/web/client.py

+        external_url: str,
+        title: str,
+        filetype: Optional[str] = None,
+        indexable_file_contents: Optional[str] = None,


good catch - fixed!

seratch · 2021-08-24T21:23:19Z

slack_sdk/web/client.py

+        https://api.slack.com/methods/files.remote.remove
+        """
+        kwargs.update({"external_id": external_id, "file": file})
+        return self.api_call("files.remote.remove", http_verb="POST", params=kwargs)

    def files_remote_share(


seratch · 2021-08-24T21:23:53Z

slack_sdk/web/client.py

-        Args:
-            channel (str): Direct message channel to close. e.g. 'D1234567890'
-        """
+    def im_close(


We can remove it in the near future but I didn't do so this time

seratch · 2021-08-24T21:25:44Z

slack_sdk/web/client.py


-    def reminders_add(self, *, text: str, time: str, **kwargs) -> SlackResponse:
+    def reminders_add(


Thanks. I just missed the one. Added with optional str type

seratch added enhancement M-T: A feature request for new functionality web-client Version: 3x labels Aug 19, 2021

seratch added this to the 3.10.0 milestone Aug 19, 2021

seratch commented Aug 19, 2021

View reviewed changes

seratch mentioned this pull request Aug 19, 2021

Add missing optional arguments slackapi/java-slack-sdk#819

Merged

6 tasks

Fix slackapi#1018 Add optional arguments in WebClient methods

9664ec1

seratch force-pushed the issue-1018-optional-args branch from 01ca6f7 to 9664ec1 Compare August 21, 2021 05:05

Rename an internal method / update dict construction

752d93e

seratch requested review from filmaj, misscoded, mwbrooks and srajiang August 24, 2021 03:29

misscoded approved these changes Aug 24, 2021

View reviewed changes

srajiang reviewed Aug 24, 2021

View reviewed changes

filmaj approved these changes Aug 24, 2021

View reviewed changes

Update some methods thanks to review feedback

512d28b

seratch commented Aug 24, 2021

View reviewed changes

Fix tests

4541e44

srajiang approved these changes Aug 24, 2021

View reviewed changes

seratch merged commit fada5dc into slackapi:main Aug 25, 2021

seratch deleted the issue-1018-optional-args branch August 25, 2021 00:18

seratch mentioned this pull request Aug 26, 2021

Fix WebClient#files_upload does not send channels since v3.10.0 #1106

Merged

16 tasks

seratch mentioned this pull request Feb 8, 2022

Fix a bug where some of the files.remote API parameters do not work since v3.10 #1173

Merged

16 tasks

		kwargs.update({"channel": channel})
		kwargs = _filter_none_values(kwargs)


		def reminders_add(self, , text: str, time: str, *kwargs) -> SlackResponse:
		def reminders_add(

Conversation

seratch commented Aug 19, 2021

Summary

Category (place an x in each of the [ ])

Requirements (place an x in each [ ])

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stasfilin commented Aug 24, 2021

Uh oh!

srajiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

filmaj left a comment

Choose a reason for hiding this comment

Uh oh!

seratch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Category (place an `x` in each of the `[ ]`)

Requirements (place an `x` in each `[ ]`)

codecov bot commented Aug 19, 2021 •

edited

Loading