-
Notifications
You must be signed in to change notification settings - Fork 847
feat: add table block #1788
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add table block #1788
Conversation
|
@hello-ashleyintech |
|
@codomposer Thanks for your contribution! taking a look now! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1788 +/- ##
==========================================
+ Coverage 83.86% 83.90% +0.03%
==========================================
Files 115 115
Lines 13049 13080 +31
==========================================
+ Hits 10944 10975 +31
Misses 2105 2105 ☔ View full report in Codecov by Sentry. |
hello-ashleyintech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
|
if everything is fine, can you merge this pr? |
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codomposer Thanks for sending this in! The code also LGTM but I'm hesitant to merge this right now with a possible API issue.
Can you let me know if you're finding something else? I believe server side changes are ongoing so hoping we can merge this soon! 🤖
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codomposer Thanks once more for these changes and conversation above! 👾
I'm finding a possible issue around raw_text in a recent commit, but believe the backend changes have since resolved. I'll explore similar changes in slackapi/node-slack-sdk#2426 but please let me know if I can help or review again!
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codomposer Thank you again for bringing improvements to reference alongside these changes! I made a few update after final testing, but think we can merge this:
- ee24a66: The max length noted in the Node SDK was in error - for now a
raw_textlimit isn't in reference so let's omit it here. - 2d4a18e: These tests are comprehensive but might not bring the confidence we hope for - I noticed these mocked incorrect responses from the API which might be outside the scope of this block addition for now.
You're super awesome for keeping up with all of these changes as we were debugging some things on the backend too. It's much appreciated! Let's merge this for next release! 🚢 💨
Summary
Closes #1729
Testing
Run all TableBlock tests
python -m pytest tests/slack_sdk/models/test_blocks.py::TableBlockTests -vRun all block tests to ensure no regressions
python -m pytest tests/slack_sdk/models/test_blocks.py -vCategory
/docs(Documents)/tutorial(PythOnBoardingBot tutorial)tests/integration_tests(Automated tests for this library)Requirements
python3 -m venv .venv && source .venv/bin/activate && ./scripts/run_validation.shafter making the changes.Contribution by Gittensor, learn more at https://gittensor.io/