-
Notifications
You must be signed in to change notification settings - Fork 68
add support for including private Slack channels in slack source #636
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
Conversation
mypy.ini
Outdated
@@ -1,5 +1,5 @@ | |||
[mypy] | |||
python_version=3.8 | |||
python_version=3.9 |
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.
is this intended?
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.
The CI was failing because a file inside the dlt package (postgres.py) uses parenthesized context managers a syntax that’s only valid in Python 3.9 and above
packages/dlt/destinations/impl/postgres/postgres.py:86: error: Parenthesized context managers are only supported in Python 3.9 and greater [syntax]
…into enh/slack_include_private_channels
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.
Looks very good!
Added a couple of minor comments
tests/slack/test_slack_source.py
Outdated
) | ||
|
||
assert expected_message_table_name in table_names | ||
assert expected_replies_table_name in table_names |
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.
Will this table be created if we have zero messages available?
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.
ah, because in three months the old messages will go away..good point. i guess we don't really need the replies test here
Co-authored-by: Violetta Mishechkina <sansiositres@gmail.com>
Co-authored-by: Violetta Mishechkina <sansiositres@gmail.com>
enh: add support for including private Slack channels in source
include_private_channels
parameter toslack_source