Conversation
d89f55f
to
992fffb
Compare
992fffb
to
e8abf82
Compare
d465f17
to
ca9dc9a
Compare
ca9dc9a
to
eac6181
Compare
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.
Overall it looks good. Pretty clean code and well documented. 👍
layabout.py
Outdated
|
||
layabout.run() | ||
""" | ||
def __init__(self, name: str, env_var: str = 'SLACK_API_TOKEN') -> None: |
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.
It would probably be good to throw a mention of the name of this environment variable into the quickstart section of the readme.
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.
And maybe a link to a page that talks about how to get such a token? I would play with it, but I'm having trouble figuring out how to get the appropriate API token.
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.
I mentioned what it defaults to in the additional API docs I've been writing on a separate branch, but you're right, it probably deserves mention in the example/README.rst
as well. I don't think it's worth mentioning how you get a token in the class-level docs, but I can include it in a list of helpful links in the Sphinx documentation sidebar.
layabout.py
Outdated
An event handler on top of the Slack RTM API. | ||
|
||
Args: | ||
name: A unique name for this :obj:`Layabout` instance. |
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.
What does the name do? Why do I care about providing it? Is it really going to be that common that I want multiple layabout instances that this needs to be a required argument?
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 the name the Slackbot will take on in it's messages in Slack?
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.
That's a great question, @RadicalZephyr. @kyle-rader's suggestion is good, but how the Slack bot is present in Slack itself is up to the developer. A bot is associated with a token, but the name, icon, etc. of the bot aren't configurable here (not easily anyway).
What I've been leaning towards is what I briefly I mentioned where I instantiate the SlackClient
:
# TODO: Maybe set an appropriate user agent string here using
# self.name.
self.slack = SlackClient(token=self._token)
The slackclient library has a method called SlackClient.append_user_agent
.
What I'm thinking I'll do now is just remove the name
argument altogether and append my own Layabout-specific user agent.
self.slack = SlackClient(token=self._token)
self.slack.append_user_agent('layabout', __version__)
tests/test_layabout.py
Outdated
) | ||
|
||
# The likelihood of this ever being a valid Slack token is probably slim. | ||
TOKEN = 'xoxb-13376661337-DeAdb33Fd15EA53fACef33D1' |
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.
you're not connecting in the tests, so why even give it a token that looks like a slack token? It's just a string. I would probably use something like 'this is not a slack token'
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.
foo_token
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.
Heh. I like 'this is not a slack token'
, so I'll probably use that. Makes sense to me.
tests/test_layabout.py
Outdated
pass | ||
|
||
layabout.handle(type='hello')(fn=handle_hello1) | ||
layabout.handle(type='hello', kwargs=kwargs)(fn=handle_hello3) |
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.
I'm confused. Can this test ever fail? It doesn't look like there are any assertions...
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.
There aren't any assertions. The test could conceivably fail if the inner workings of Layabout
were screwed up enough, but it's unlikely.
We actually had a separate conversation about this test in particular where you thought I was being overly specific. The original test did include assertions, but became troublesome. I was registering handlers and then reaching into the private self._handlers
object to verify they were present in the right way. I was also verifying that the functions that had been registered were exactly the same as their definitions, but that broke down because of the decorators wrapping and changing their id
s.
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.
Oh, that makes sense. Then at this point, this test is pretty much just an executed example of different ways to register event handlers. In that case, I might put in an assertion that there are 4 handlers registered. To me that would communicate a bit better what the test is about, though it doesn't really change what it's testing. Either way is fine though.
tests/test_layabout.py
Outdated
|
||
rtm_connect = MagicMock(side_effect=connections) | ||
rtm_read = MagicMock(return_value=events) | ||
slack = MagicMock(rtm_connect=rtm_connect, rtm_read=rtm_read) |
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.
This fake slack client setup is appearing a lot. I don't know anything about MagicMock
but I would maybe try to simplify this setup so it's more expressive of what the fake slack should do and less about the mock setup. At the very least, in quite a few places you're providing stubs for rtm_connect
and rtm_read
so this line in particular feels like noise.
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.
Aye 👍 for re-usable testing resources.
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.
I see what you guys are saying and I agree this can (and should) be simplified, but I don't see an easy way to do it yet. There are a lot of similarities, but some differences between setups and I need them to be configurable, so a pytest.fixture
doesn't seem quite right here. Perhaps I can craft an auxiliary function like
def mock_slack_client(connections, events):
...
and use it like
SlackClient = mock_slack_client([False, True], [dict(...), dict(...)])
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.
I'm a fan of using your codes setup functions to build the required objects.
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.
I don't follow. In this case, I don't have a setup function that builds a fake SlackClient
...
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.
Your concept of a mock_slack_client
helper is pretty much what I was thinking, though I would use keyword arguments so it's clearer what it's doing at the call site. The idea is just to condense the setup but retain the useful bits of information that make each test setup unique.
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.
I'm not really sure what I meant when I wrote that. =P
I think I was thinking about stating that configurable objects should have instantiation done in the code base and you should use the codes setup functions but that doesn't make any sense in the context of mocks.
tests/test_layabout.py
Outdated
all_calls = [call_hello, call_goodbye] | ||
handle_hello.assert_called_once_with(*call_hello[1], **call_hello[2]) | ||
handle_goodbye.assert_called_once_with(*call_goodbye[1], **call_goodbye[2]) | ||
handle_splat.assert_has_calls(all_calls) |
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.
I know the set up for some of these tests is necessarily complex, but these tests are the size where my eyes glaze over when I look at them.
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.
I agree and to build off that I would try to make fewer (preferably 1) assertion per test and use shareable testing resources to be able to setup your fixtures/mocked objects in like 1-2 lines.
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.
These tests in general or just this particular one? If we're talking about test_layabout_can_handle_events
then yeah, I could probably break it up into smaller pieces. The others generally have 1 or 2 assertions, but this one has a lot.
I think I can break the *
handling test out into a separate test, but I felt like in order to adequately test normal event handling I needed to have more than one handler registered so I could verify they weren't accidentally called for the wrong events.
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.
These tests all are pretty big to me:
test_layabout_can_reuse_an_existing_client_to_reconnect
test_layabout_can_continue_after_successful_reconnection
test_layabout_can_handle_events
Though part of that "bigness" is the testing setup duplication, which was commented on previously, and the number of newlines to keep the lines short. On closer inspection, the tests themselves aren't doing a ton, except can_handle_events
.
In general, I agree with @kyle-rader's advice to try and keep the tests to one assertion per test. Although, I would expect to see at least one comprehensive test like this that has more assertions, I would expect it to be accompanied by smaller tests that break out each individual piece of functionality. For instance, if I had test driven this event handling structure, I would have done it something like this:
- register one handler for a specific event, assert it's called
- register one handler for a specific event, assert that it isn't called when a different event is triggered
- register two handlers for the same event, trigger one, assert both are called
- register two handlers for different events, trigger one, assert the correct one is called
- register a catchall handler, trigger an event, assert it's called
- register a catchall handler, trigger several different events, assert it's called the correct number of times
- register a specific handler and a catchall, trigger the registered event, assert both are called
Now, that's quite a few tests for event triggering. In order to make writing and reading them more palatable, you'd want to minimize the size of the setup (ideally 1 line). This probably means having a fixture function for creating the mock slack client like we discussed above, and not using the decorated def
style registration. It would be fine to have a group of handler functions defined at the top level, and then register the needed ones explicitly as needed in each test.
Finally, I think it would be helpful to have a concise idiom for the assertions that lets you make each check in one line as well. One that pops to mind is having the handlers side-effect modify a list, inserting the handler name into it. Then each assertion is pretty self-documenting, and if it fails, the problem should be immediately obvious.
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.
@reillysiemens
Some over-arching questions:
- What is the
name
for, I never see it used outside a ToDo block. - I see a theme of breaking code up into smaller more distinct single responsibility functions. Both in the client and the tests.
layabout.py
Outdated
|
||
Args: | ||
name: A unique name for this :obj:`Layabout` instance. | ||
env_var: The environment variable to try to load a Slack API token |
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.
It would be good to indicate the environment variable indirection in the name of this somehow instead of env_var
.
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.
env_var_name
is probably not a bad name for this one, but I don't think there's really a lot of room for confusion here either and I prefer the brevity of env_var
. There's precedent for this naming scheme as well. The venerable Python CLI library, Click, calls this envvar
.
At the end of the day, if I declare an environment variable in my shell as
export SLACK_API_TOKEN='this is not a slack token'
then the environment variable is $SLACK_API_TOKEN
and we can just refer to it as an environment variable. Which environment variable do I want? Oh, $SLACK_API_TOKEN
.
That said, I could be sold on emphasizing the relationship to the token by calling this something like token_env_var
. I may actually do that.
layabout.py
Outdated
An event handler on top of the Slack RTM API. | ||
|
||
Args: | ||
name: A unique name for this :obj:`Layabout` instance. |
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 the name the Slackbot will take on in it's messages in Slack?
layabout.py
Outdated
|
||
# We don't have an existing token, so let's try to get one from an | ||
# environment variable or give up. | ||
if self._token is None: |
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.
It would be good to break this acquisition of a token into a function. One of the guidelines we follow writing objects in Ruby is Sandi Metz - 5 lines or less per function rule.
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.
Then you could perhaps set the default token value of the arg to this function equal to calling the getToken
function - That's how I would approach in Ruby, idk if that is possible here.
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. I think what you're talking about would need to be accomplished through a @property
in Python.
class Layabout:
def __init__(self, env_var='SLACK_API_TOKEN'):
self._token = None
self._env_var
@property
def token(self):
return self._token or os.getenv(self._env_var)
@token.setter
def token(self, value):
self._token = value
This way you could do
>>> layabout = Layabout()
>>> layabout.token # 'SLACK_API_TOKEN' is undefined.
None
>>> layabout.token = 'this is not a slack token'
>>> layabout.token
'this is not a slack token'
>>> layabout.token = None # Reset the token.
>>> os.environ['SLACK_API_TOKEN'] = 'this might be a slack token'
>>> layabout.token
'this might be a slack token'
This is powerful, but I also generally avoid it because I believe it violates the Zen of Python mantra of
Simple is better than complex.
and it can be especially surprising when what you thought was a simple attribute lookup ends up executing a whole mess of stuff.
However, in this case, if I set the inner token
as self.__token
and create a @property
of _token
, then it's still "private" and can be left as an implementation detail giving me all of the power and none of the responsibility (muahaha!). Thoughts, @RadicalZephyr?
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 you have the time, I'm curious about your thoughts here as well, @rawrgulmuffins.
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.
I don't think it needs to be a property. Seems to me, you could leave the default token value as None
, then do something like
if token:
self.slack = None
self._token = token or self.get_token()
IMO, I think the main reason to introduce a property is when you had an attribute as part of your public API, and you want to change it to be a function call without breaking code.
layabout.py
Outdated
|
||
# Either we've never connected before or we're purposefully resetting | ||
# the connection. | ||
if self.slack is None or resetting: |
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.
This could also be a function like ensureSlackClient
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.
I see the value of the getToken
function, but I feel like this might be taking it too far. Once you remove the TODO
below this this is honestly just two lines. ¯\_(ツ)_/¯
I guess if you were to take into account features I've discussed about like adding a user agent to the Slack client, then maybe this would be useful. 🤔
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.
I like this idea, because naming it in a function would get rid of the need for your comment I think.
A comment is an apology for not choosing a more clear name, or a more reasonable set of parameters, or for the failure to use explanatory variables and explanatory functions.
With this, and the above change about adding a get_token
function, 24 lines of comments, code and whitespace would become 5 lines of code. Seems like a win to me.
layabout.py
Outdated
# TODO: Should retries start at 0 or 1? | ||
for retry in range(retries): | ||
if self.connect(): | ||
return True |
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.
It's not clear from reading if _reconnect
and connect
have the same return type or not. connect
returns the call to rtm_connect()
and here we return a Boolean. Does rtm_connect()
also return a Boolean?
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.
As denoted by their type annotations, both methods have the same return type:
connect
def connect(self, token: str = None) -> bool: ...
_reconnect
def _reconnect(self, retries: int,
backoff: Callable[[int], float]) -> bool: ...
The SlackClient.rtm_connect
method eats any exceptions and always returns a bool
.
tests/test_layabout.py
Outdated
pass | ||
|
||
@layabout.handle('hello') | ||
def handle_hello2(slack, event): |
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.
Are hello2
and hello4
ever used?
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.
In this test being decorated is being used.
@layabout.handle('hello')
def handle_hello2(slack, event): ...
is meant to verify that nothing explodes when a handler is registered via decorator. Likewise,
@layabout.handle('hello', kwargs=kwargs)
def handle_hello4(slack, event): ...
is meant to verify that nothing explodes when a handler is registered via decorator with keyword arguments.
I could break all 4 of these cases up into separate tests without issue. Mainly, as long as an exception isn't thrown then these functions ought to have been registered properly and the test should pass.
tests/test_layabout.py
Outdated
""" | ||
layabout = Layabout(name='test') | ||
|
||
def invalid_handler1(slack): |
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 naming of function with n
appended to end seems odd to me. If a function is different enough to get a new n
at the end, is it not different enough to get a meaningful name variation?
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.
Not really, IMHO. They're both invalid handlers and they're really invalid in the same way. If anything I'd be inclined to name them the same thing.
def insufficient_args(slack):
pass
with pytest.raises(TypeError) as func_exc:
layabout.handle(type='hello')(fn=insufficient_args)
with pytest.raises(TypeError) as decorator_exc:
@layabout.handle('hello')
def insufficient_args(slack):
pass
I could also just shorten the first one to
with pytest.raises(TypeError) as func_exc:
layabout.handle(type='hello')(fn=lambda s: ...)
tests/test_layabout.py
Outdated
monkeypatch.setattr(os, 'environ', environ) | ||
monkeypatch.setattr('layabout.SlackClient', SlackClient) | ||
|
||
# Purposefully don't provide a token so we have to use an env var. |
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.
This comment might connect with the following line more if the following line passed token=None
, but that might break the scenario you are trying to test.
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.
Nah, that'd be just fine. It's more explicit and that's OK by me.
tests/test_layabout.py
Outdated
|
||
rtm_connect = MagicMock(side_effect=connections) | ||
rtm_read = MagicMock(return_value=events) | ||
slack = MagicMock(rtm_connect=rtm_connect, rtm_read=rtm_read) |
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.
Aye 👍 for re-usable testing resources.
tests/test_layabout.py
Outdated
all_calls = [call_hello, call_goodbye] | ||
handle_hello.assert_called_once_with(*call_hello[1], **call_hello[2]) | ||
handle_goodbye.assert_called_once_with(*call_goodbye[1], **call_goodbye[2]) | ||
handle_splat.assert_has_calls(all_calls) |
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.
I agree and to build off that I would try to make fewer (preferably 1) assertion per test and use shareable testing resources to be able to setup your fixtures/mocked objects in like 1-2 lines.
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.
I'm not one of the project maintainers so I don't plan on giving an explicit approve or request changes on this one.
:alt: Unix build status on Travis CI | ||
|
||
.. image:: https://img.shields.io/coveralls/reillysiemens/layabout/master.svg?style=flat-square&label=coverage | ||
:target: https://coveralls.io/github/reillysiemens/layabout?branch=master |
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 there a way to have the branch target the current selected branch on github?
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.
No, not to my knowledge. That would be great and I searched for one briefly, but didn't find it. How would such a thing work anyway? I think it'd have to dynamically change this file based on the branch, which wouldn't be great.
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.
It would have to be a keyword or special sequence that signaled "use the http reference url to find the branch name for this request."
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.
You can make a PR if you want. 😜
layabout.py
Outdated
|
||
from slackclient import SlackClient | ||
|
||
# TODO: This is a dependency of slackclient needed for exception handling. In |
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.
Why not pull it through slackclient then?
Also, this probably needs an explanation for why it's required and how you think it can be removed.
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.
Oh! Yes, I should be pulling it through slackclient
. I always forget that's an option! There's an explanation of why next to where I use the import from this library rather than at the import site.
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.
It looks like they were really careful not to re-export it anywhere. I can't find an easy way to pull it through slackclient
... 😞
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.
That's kinda dumb. =(
layabout.py
Outdated
layabout = Layabout('app') | ||
|
||
|
||
@layabout.handle('message') |
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.
It would probably be useful to someone like me that hasn't looked at the slack events page to have a link to the full page.
I know that figuring out what document to read to find all the valid events for an API can sometimes be a pain in the ass.
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.
I fully intend on linking to helpful Slack API resources from the main Sphinx documentation, but I didn't think it was necessary to link to it from this docstring describing the class. If anything I'd put it in the Layabout.handle
docstring. Yeah... I suppose I can do that.
layabout.py
Outdated
layabout.run() | ||
""" | ||
def __init__(self, name: str, env_var: str = 'SLACK_API_TOKEN') -> None: | ||
# TODO: Consider keyword only arguments. |
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 by keyword only arguments you mean **kwargs
then I'm not a fan. I prefer to have the explicit signature where possible.
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.
No, I mean keyword-only arguments from PEP 3102 to enforce that users can only call these methods with keyword arguments rather than positional arguments, thus ensuring a greater level of backwards compatibility if I chose to do something like change the order of arguments.
In this case that might look something like this (note the *
in the signature):
class Layabout:
def __init__(self, *, name: str, env_var: str = 'SLACK_API_TOKEN') -> None:
...
then callers would encounter a TypeError
unless they specifically used keyword arguments.
>>> layabout = Layabout()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: __init__() missing 1 required keyword-only argument: 'name'
>>> layabout = Layabout('app')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: __init__() takes 1 positional argument but 2 were given
>>> layabout = Layabout(name='app')
Later, I could then decide that I wanted name
to be optional and env_var
to come first in the signature
class Layabout:
def __init__(self, *, env_var: str = 'SLACK_API_TOKEN', name: str = None) -> None:
and it wouldn't matter at all since all users would have been prohibited from writing code that would break backwards compatibility here.
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.
I'd totally forgotten about this feature.
I think I personally lean on the side of keyword only arguments being best used for optional arguments. I think it's less clear that name is a required argument from the signature.
layabout.py
Outdated
self.name = name | ||
self._env_var = env_var | ||
self._token: str = None | ||
self.slack: SlackClient = None |
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.
My brain says this should either be assignable or a _
attribute. Probably the inverse for _env_var.
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.
I originally had it as _private
, but when I realized that I was already allowing users direct access to it via handlers
@layabout.handle('hello')
def hello(slack, event):
slack.server.websocket.close() # Oh, snap! Now it's all broken!
I didn't see any reason not to make it part of the public interface. On top of that, making it part of the public interface highlighted the possibility that a user might want to use it before entering the event loop to do some actions they didn't want to be doing repeatedly. This is also why I've currently made the Layabout.connect
method public, but as I mentioned earlier, I'm a little conflicted about it.
Is that use case enough of a reason to expose another public method when I'm gunning for a ruthlessly simple library here? I'm not sure. 🙃
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.
Thoughts, @kyle-rader and @RadicalZephyr?
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.
🤔
Okay, how about this paradigm instead. Don't expose the connect
method, or the slack client (except in the handler functions). If the user wants to connect to slack and do stuff before starting the layabout event loop, let them set it up themselves using the SlackClient
class directly. Then, let them pass that same SlackClient
into their layabout
instance, either at construction time, or with an attribute setter. Using a setter would let them still use the decorator style event registration without the slack client needing to be constructed globally too.
That would also have the side benefit that it would give you a way to inject a mock into your test Layabout
without monkeypatching.
The only downside is that they lose the ability to use your environment variable token retrieval. But that's really just a convenience for making the simple case usage as simple as possible, so it's really not a loss to someone who wants to have more control.
layabout.py
Outdated
raise FailedConnection('Failed to connect to the Slack API') | ||
|
||
# TODO: Should we force callers to handle KeyboardInterrupt on their | ||
# own, or should we try to handle it for them? 🤔 |
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.
You should let them handle it. If they have any system side effects (files, environment variables, child processes) you'll potentially cause them to leak those resources.
I would personally go so far as to say that if you need to catch a KeyboardInterrupt
then you should re-raise it after.
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.
I appreciate your answer, thanks. This is what I was leaning towards, but now I actually have some solid rationale. 😁
layabout.py
Outdated
|
||
if not until(events): | ||
# TODO: Is this even a good debugging message? | ||
log.debug('Terminal condition met') |
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.
No. I have no idea what it's trying to say or why it's here.
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.
Eh, it's mostly here just to log why we've exited the Layabout event loop early. Imagine the following:
>>> import logging
>>> from layabout import Layabout
>>> logging.basicConfig(level=logging.DEBUG)
>>> logger = logging.getLogger('app')
>>> layabout = Layabout('app')
>>> def not_even_once(events):
... logger.debug("Found these events: %s", events)
... return False
...
>>> layabout.run(until=not_even_once)
DEBUG:urllib3.connectionpool:Starting new HTTPS connection (1): slack.com
DEBUG:urllib3.connectionpool:https://slack.com:443 "POST /api/rtm.start HTTP/1.1" 200 None
DEBUG:app:Encountered these events: []
DEBUG:layabout:Terminal condition met
>>>
Maybe it would be more helpful if it was
Terminal condition met. Exiting event loop
or perhaps it's just so obvious that it's altogether unnecessary... 🤔
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.
That makes much more sense. Even in the context of the other log lines the Terminal condition met phrase doesn't make a whole lot of sense to me.
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.
It's slightly better with "Exiting event loop". I think that statement alone is probably a better log message. If the user wants their terminal condition function to log something when it returns False
they can do that themselves.
layabout.py
Outdated
# Handle events! | ||
for event in events: | ||
type_ = event.get('type') | ||
# TODO: Should * handlers be run first? |
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.
I don't know if they should be handled first but there should definitively be an ordering of some kind (instead of the source code ordering which is going to be first handle call first).
My gut says that * should be handled last since they're most likely to fail?
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.
Are they the most likely to fail? I don't see any reason why they should be.
When you say a definitive ordering do you mean there should be a priority ordering that's user-configurable? Something like a CSS z-index
?
By
... first handle call first ...
I assume you're calling attention to the fact that the order of handler registration determines the order in which handlers are called. If that's explicitly documented (which it's currently not) what's the problem with that?
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.
Are they the most likely to fail?
Having to handle all of the different kinds of input in a single function seems more complex to me. Though on second thought hopefully people aren't writing handlers that will stack trace.
When you say a definitive ordering do you mean there should be a priority ordering that's user-configurable? Something like a CSS z-index?
Probably overkill. Potentially I could see adding a sorting callback for the user to add their own ordering.
If that's explicitly documented (which it's currently not) what's the problem with that?
That's a totally fine answer to me.
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.
I think having handler registration order be call order is pretty reasonable once it's documented. Then if the user desires, they can specify the call order without any further explicit support in Layabout
.
And if for some reason they need their function definitions laid out in a different order than they want the handler call order, they can just not use the decorator and call handle
explicitly in the order they need.
It seems to me the added complexity of providing an explicit ordering mechanism is not worthwhile. Not without a good use case certainly.
I don't really have an opinion on whether *
handlers should go first or last. I think either choice is going to be problematic for certain uses. That might be an argument for having pre-*
and post-*
handlers, but I don't think it's necessary right now. Again, I would want a good motivating use case first.
tests/test_layabout.py
Outdated
def handle_hello1(slack, event): | ||
pass | ||
|
||
@layabout.handle('hello') |
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.
Oooo, OOO. I didn't realize you didn't do any value verification for event names.
So you can enter event names that are invalid and an error won't be thrown?
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.
Yeah, I purposefully didn't do any value verification for event names so I could be responsive to Slack updating their API without me updating this library.
New Slack RTM API event? Boom! Look at that. It's already supported out of the box from day one.
This does, of course, mean that you can enter invalid event names and an error won't be thrown, but there's little harm as you'll just never encounter such an event and the handler won't even be called.
Slack's own slackclient
library doesn't validate their SlackClient.api_call
method (presumably for the same reasons), so why should I?
This provides the implementation and code coverage for Layabout. There are a few unanswered questions marked with
TODO
s that I'd like to resolve before officially publishing this. I'm also interested in feedback on the design now in this early stage, so anything is fair game.In particular I'm curious about whether my very recent decision to make the
connect
method part of the public interface is a good idea. Previously I just hadhandle
andrun
, but once I made theslack
attribute containing theslackclient.SlackClient
instance public it didn't seem right to not allow users touse it right away before entering into the event loop.
If you have the time I highly recommend installing the library via from this branch and playing around with it to get a feel for it.