Added support for rtm.connect. Fixes #190 by Roach · Pull Request #209 · slackapi/python-slack-sdk

Roach · 2017-08-17T23:19:41Z

Summary

Added support for rtm.connect. Fixes #190

With rtm.start:
Print channel count (should be 13)

>>> if sc.rtm_connect():
...     while True:
...         event = sc.rtm_read()
...         print(len(sc.server.channels))
...         time.sleep(1)
... else:
...     print("Connection Failed")
... 
13

With rtm.connect:
Print channel count (should be 0)

>>> if sc.rtm_connect(with_team_state=False):
...     while True:
...         event = sc.rtm_read()
...         # Print channel count (should be 0)
...         print(len(sc.server.channels))
...         time.sleep(1)
... else:
...     print("Connection Failed")
... 
0

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.

Roach

Accidentally left some lines commented out, fixing.

aoberoi · 2017-08-17T23:22:38Z

+rtm.start vs rtm.connect
+---------------------------
+
+If you expect your app to be used on large teams, we recommend using starting the RTM client with `rtm.connect` rather than `rtm.start`, which is the default connection method for this client.


recommedation: ... rather than the default connection method for this client, rtm.start.

when the code will be checked in?

aoberoi

mostly looks good... but the age-old question: tests? it doesn't look like we have a very deep coverage right now, but i think for new changes we should aim to improve that situation.

aoberoi · 2017-08-17T23:25:42Z

+`rtm.connect` provides a lighter initial connection payload, without the team's channel and user information included. You'll need to request channel and user info via
+the Web API separately.
+
+To do this, simply pass `with_team_state=False` into the `rtm_connect` call, like so:


the word "this" is ambiguous and might refer to "requesting channel and user info via the Web API". maybe say "To get a lighter connection payload, ..."

aoberoi · 2017-08-17T23:27:32Z

          time.sleep(1)
  else:
-      print "Connection Failed, invalid token?"
+      print "Connection Failed"


uhh.. this directory is supposedly ignored, you're still committing files here? might want to git rm -rf docs/_sources to get the actual delete as part of the commit

ditto for the docs/.doctrees directory. seems like something that should be ignored too.

aoberoi · 2017-08-17T23:29:14Z

        :Args:
-            None
+            with_team_state (bool): Connect via `rtm.start` to pull workspace state information.
+            `false` connects via `rtm.connect, which is lighter weight and better for very large teams.


capital F in False?

ending backtick after rtm.connect

aoberoi · 2017-08-17T23:30:03Z

+        self.server.rtm_connect(use_rtm_start=with_team_state)
+        return True
+        # except:
+        #     return False


why not just delete these lines?

They weren't supposed to stay commented out 😐

aoberoi · 2017-08-17T23:36:55Z

so for the lines you just uncommented, if we actually removed them, wouldn't that also solve #181? jeez we should really have some tests. 😢

codecov · 2017-08-18T00:14:51Z

Codecov Report

Merging #209 into master will decrease coverage by 0.07%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
- Coverage   61.84%   61.76%   -0.08%     
==========================================
  Files           9        9              
  Lines         304      306       +2     
  Branches       56       57       +1     
==========================================
+ Hits          188      189       +1     
- Misses        102      103       +1     
  Partials       14       14

Impacted Files	Coverage Δ
slackclient/server.py	`61.66% <40%> (-0.2%)`	⬇️
slackclient/client.py	`38.88% <50%> (ø)`	⬆️

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 24dec80...775b091. Read the comment docs.

Roach · 2017-08-18T21:24:31Z

@aoberoi it would, but if a dev is explicitly checking if rtm_connect is True as opposed to just seeing if it's truthy, it'd be a breaking change... 🤔

aoberoi · 2017-08-18T21:49:22Z

ah, thanks for pointing that out @Roach.

we spoke about it in person and realized we can still fix the issue regarding the visibility of thrown exceptions (by not catching them and returning False) while leaving the return type the same (so that successful connections still return True).

we should pop back into conversations where people wanted the entire rtm.start Web API payload and direction them to inside the rtm.server Python object.

Roach · 2017-08-23T15:38:34Z

Merging this with tests coming in a future release in order to resolve a customer issue.

rkbala · 2017-08-23T17:09:59Z

I am getting the below error:

TypeError: rtm_connect() got an unexpected keyword argument 'with_team_state'

* Reverting #209 * Rebuilt docs to remove 'rtm.connect' references

* Added rtm.connect support * updated docs * updated gitignore * Uncommented lines which were accidentally left commented * Updated docs * Removed ignored doc source files * Fixed typo in method docs * fixed line length * updated rim start tests * regenerated docs

* Reverting slackapi#209 * Rebuilt docs to remove 'rtm.connect' references

Roach added 3 commits August 17, 2017 15:37

Added rtm.connect support

4382c22

updated docs

6a9c2e7

updated gitignore

25e9daa

Roach commented Aug 17, 2017

View reviewed changes

aoberoi reviewed Aug 17, 2017

View reviewed changes

Roach added 2 commits August 17, 2017 16:24

Uncommented lines which were accidentally left commented

f95bdf6

Updated docs

8b9a1c4

aoberoi reviewed Aug 17, 2017

View reviewed changes

Roach added 3 commits August 17, 2017 16:47

Removed ignored doc source files

cdea57e

Fixed typo in method docs

e445191

fixed line length

f3684ec

Roach added 2 commits August 18, 2017 14:56

updated rim start tests

b20226f

regenerated docs

775b091

aoberoi mentioned this pull request Aug 22, 2017

Allow use of custom parameters on rtm_connect() #210

Merged

2 tasks

Roach merged commit 6d2fa8c into master Aug 23, 2017

Roach added a commit that referenced this pull request Aug 23, 2017

Reverting #209

54cefff

Roach mentioned this pull request Aug 23, 2017

Revert #209 #212

Merged

2 tasks

Roach added a commit that referenced this pull request Aug 23, 2017

Revert #209 (#212)

d83bb74

* Reverting #209 * Rebuilt docs to remove 'rtm.connect' references

Roach added this to the 1.x milestone Dec 2, 2017

c-goosen pushed a commit to c-goosen/python-slackclient that referenced this pull request Jun 18, 2019

Revert slackapi#209 (slackapi#212)

f900bc2

* Reverting slackapi#209 * Rebuilt docs to remove 'rtm.connect' references

Uh oh!

Conversation

Roach commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Requirements (place an x in each [ ])

Uh oh!

Roach left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

aoberoi commented Aug 17, 2017

Uh oh!

codecov Bot commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Roach commented Aug 18, 2017

Uh oh!

aoberoi commented Aug 18, 2017

Uh oh!

Roach commented Aug 23, 2017

Uh oh!

rkbala commented Aug 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Roach commented Aug 17, 2017 •

edited

Loading

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

Roach left a comment •

edited

Loading

codecov Bot commented Aug 18, 2017 •

edited

Loading