Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix type hint for event constraint to allow None subtypes #616

Merged
merged 2 commits into from
Mar 17, 2022
Merged

fix type hint for event constraint to allow None subtypes #616

merged 2 commits into from
Mar 17, 2022

Conversation

alexrashed
Copy link
Contributor

This PR adjusts the type hints for event constraints such that the type hint explicitly allows None for the constraint.
The additional optional represents the check implemented here:

if expected_subtype is None:
# "subtype" in constraints is intentionally None for this pattern
return "subtype" not in event_payload

This change allows a type-safe usage of the explicit None subtype constraint:

@app.event(
    event={
        "type": "message",
        "subtype": None
    }
)
def handle_messages_without_subtype(client: WebClient, event: dict) -> None:
    ...

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

  • slack_bolt.App and/or its core components
  • slack_bolt.async_app.AsyncApp and/or its core components
  • Adapters in slack_bolt.adapter
  • Document pages under /docs
  • Others: builtin matchers

Requirements (place an x in each [ ])

Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.

  • 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 ./scripts/install_all_and_run_tests.sh after making the changes.

PS.: Thanks for the awesome framework, it really is fun to use! 馃挴
PPS.: The CLA tooling described in the contribution guideline doesn't really work for me (couldn't select anything), but I explicitly accepted the conditions by signing off the commit.

Signed-off-by: Alexander Rashed <alexander.rashed@localstack.cloud>
@alexrashed alexrashed changed the title fix type hint for message constraint to allow None subtypes fix type hint for event constraint to allow None subtypes Mar 17, 2022
@seratch seratch self-assigned this Mar 17, 2022
@seratch seratch added bug Something isn't working area:async area:sync labels Mar 17, 2022
@seratch seratch added this to the 1.12.1 milestone Mar 17, 2022
Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexrashed, thanks for making this improvement! The changes on the docs and code are fine but can you revert the changes under docs/api-docs/? We will generate the api-docs when making a new release.

@seratch
Copy link
Member

seratch commented Mar 17, 2022

@alexrashed

PS.: Thanks for the awesome framework, it really is fun to use! 馃挴

Thank you! I am so glad to hear this 馃槂

PPS.: The CLA tooling described in the contribution guideline doesn't really work for me (couldn't select anything), but I explicitly accepted the conditions by signing off the commit.

The CLA Assistant service might have some issues when you accessed. Can you try visiting https://cla-assistant.io/slackapi/bolt-python again and signing our CLA there with the email address that you used for the commit in this PR?

Signed-off-by: Alexander Rashed <alexander.rashed@localstack.cloud>
@CLAassistant
Copy link

CLAassistant commented Mar 17, 2022

CLA assistant check
All committers have signed the CLA.

@alexrashed
Copy link
Contributor Author

Thanks for the fast response! After some retries signing the CLA worked and I reverted the changes to the auto-generated docs.

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me. After merging this PR., I will apply the corresponding change to Japanese document page.

@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #616 (ea02b45) into main (54114ca) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #616   +/-   ##
=======================================
  Coverage   91.46%   91.46%           
=======================================
  Files         170      170           
  Lines        5766     5766           
=======================================
  Hits         5274     5274           
  Misses        492      492           
Impacted Files Coverage 螖
slack_bolt/app/app.py 87.69% <酶> (酶)
slack_bolt/app/async_app.py 94.19% <酶> (酶)
slack_bolt/listener_matcher/builtins.py 93.83% <100.00%> (酶)

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 54114ca...ea02b45. Read the comment docs.

@seratch seratch merged commit 851376e into slackapi:main Mar 17, 2022
@alexrashed alexrashed deleted the fix-message-subtype-type-hints branch March 17, 2022 10:01
seratch added a commit that referenced this pull request Mar 17, 2022
@seratch
Copy link
Member

seratch commented Mar 17, 2022

@alexrashed Thanks for your contribution 馃帀

@seratch seratch modified the milestones: 1.12.1, 1.13.0 Mar 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:async area:sync bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants