Fix #1127 Add more docstrings to slack_sdk.models classes by seratch · Pull Request #1135 · slackapi/python-slack-sdk

seratch · 2021-11-15T06:27:25Z

Summary

This pull request fixes #1127 by adding docstring to the Block Kit model classes. While working on this, I've noticed there are a few unusable arguments in some classes. I've removed them as well.

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-11-15T06:28:09Z

slack_sdk/models/blocks/block_elements.py


    attributes = {"type", "action_id", "placeholder", "confirm"}

-    def _subtype_warning(self):


This is already defined in the super class. We can safely remove this overridden one

seratch · 2021-11-15T06:28:43Z

slack_sdk/models/blocks/block_elements.py

        self,
        *,
        action_id: Optional[str] = None,
-        placeholder: Optional[str] = None,


Button block element does not accept placeholder. This has been wrong for a long time.

seratch · 2021-11-15T06:29:12Z

slack_sdk/models/blocks/block_elements.py

        """
        This select menu will load its options from an external data source, allowing
        for a dynamic list of options.
-        https://api.slack.com/reference/block-kit/block-elements#external-select


The document URL was wrong

seratch · 2021-11-15T06:29:34Z

slack_sdk/models/blocks/block_elements.py

        *,
        action_id: Optional[str] = None,
        placeholder: Optional[Union[str, dict, TextObject]] = None,
-        confirm: Optional[Union[dict, ConfirmObject]] = None,


Input block element does not accept confirm object. This has been wrong for a long time.

seratch · 2021-11-15T06:29:45Z

slack_sdk/models/blocks/block_elements.py

        self,
        *,
        action_id: Optional[str] = None,
-        placeholder: Optional[Union[str, dict, TextObject]] = None,


Radio buttons block element does not accept placeholder. This has been wrong for a long time.

seratch · 2021-11-15T06:30:14Z

tests/slack_sdk/models/test_elements.py

                text=STRING_301_CHARS, action_id="button", value="click_me"
            ).to_dict()

+    def test_action_id_length(self):


This is implemented in its super class. Just in case, I've added more tests for it.

codecov · 2021-11-15T06:35:49Z

Codecov Report

Merging #1135 (e2a1ce6) into main (cf22599) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1135      +/-   ##
==========================================
- Coverage   86.62%   86.60%   -0.03%     
==========================================
  Files         110      110              
  Lines       10749    10747       -2     
==========================================
- Hits         9311     9307       -4     
- Misses       1438     1440       +2

Impacted Files	Coverage Δ
slack_sdk/models/blocks/basic_components.py	`87.12% <ø> (ø)`
slack_sdk/models/blocks/block_elements.py	`92.51% <ø> (+0.20%)`	⬆️
slack_sdk/models/blocks/blocks.py	`96.26% <ø> (ø)`
slack_sdk/socket_mode/builtin/internals.py	`70.66% <0.00%> (-1.34%)`	⬇️

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 cf22599...e2a1ce6. Read the comment docs.

filmaj

I love the smell of docs first thing Monday morning. Thanks for doing this, looks great! Left one tiny comment.

filmaj · 2021-11-15T17:50:36Z

slack_sdk/models/blocks/block_elements.py

+                Should be unique among all other action_ids in the containing block.
+                Maximum length for this field is 255 characters.
+            options (required): An array of option objects. A maximum of 10 options are allowed.
+            initial_options: An array of option objects that exactly matches one or more of the options within options.


I see that this text is taken from the checkboxes docs, but because we lack linking like the docs site has, some of the context is lost here. May I suggest we expand this a bit to make it clear which "options" are being talked about within the sentence (we are both talking about the options argument as well as the concept of options!)?

Suggested change

initial_options: An array of option objects that exactly matches one or more of the options within options.

initial_options: An array of option objects that exactly matches one or more of the options provided to the `options` argument.

This is a good point. Indeed, the sentence here does not make sense 😓 Although I don't want to have so many edits on these (as these are taken from the API documents) but will adjust some parts like this.

mwbrooks

Nice addition! I only found one small typo and left a suggestion for it.

mwbrooks · 2021-11-15T22:16:20Z

slack_sdk/models/blocks/blocks.py

+                block_id should be unique for each message or view and each iteration of a message or view.
+                If a message or view is updated, use a new block_id.
+            hint: An optional hint that appears below an input element in a lighter grey.
+                It must be a a text object with a type of plain_text.


Suggested change

It must be a a text object with a type of plain_text.

It must be a text object with a type of plain_text.

Thanks. Perhaps, I need to send a patch to the original document as well.

@seratch Ah, I didn't realize you borrowed it directly from api.slack.com. I can send a patch for the original documentation and add you as a reviewer 👍🏻

Fixed a a text object on https://api.slack.com/reference/block-kit/blocks#input_fields

@mwbrooks You are so quick 🚀 Thank you!

Fix slackapi#1127 Add more docstrings to slack_sdk.models classes

07c5696

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

seratch added this to the 3.12.0 milestone Nov 15, 2021

seratch requested review from filmaj, srajiang and stevengill November 15, 2021 06:27

seratch self-assigned this Nov 15, 2021

seratch commented Nov 15, 2021

View reviewed changes

filmaj approved these changes Nov 15, 2021

View reviewed changes

mwbrooks approved these changes Nov 15, 2021

View reviewed changes

Fix

e2a1ce6

seratch merged commit 1de0d2a into slackapi:main Nov 19, 2021

seratch deleted the issue-1127-models-docstring branch November 19, 2021 08:57


		attributes = {"type", "action_id", "placeholder", "confirm"}

		def _subtype_warning(self):

	initial_options: An array of option objects that exactly matches one or more of the options within options.
	initial_options: An array of option objects that exactly matches one or more of the options provided to the `options` argument.

	It must be a a text object with a type of plain_text.
	It must be a text object with a type of plain_text.

Conversation

seratch commented Nov 15, 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!

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

Uh oh!

Codecov Report

Uh oh!

filmaj 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!

mwbrooks 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

codecov bot commented Nov 15, 2021 •

edited

Loading