Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
77 multiple commands from a single message #82
77 multiple commands from a single message #82
Changes from 7 commits
79319e3
7887f41
b295f36
2dce8c9
48a3c40
f3cd583
0edb090
e6bce1d
c2e3964
1328d5f
280cc57
9906aa4
d89b586
74e0227
8b919dc
b1ae0e6
aa5400a
978ad89
ee61b9b
a60c8be
125c6f4
a0384bc
2a0ad1e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
_collect_responses
might be a nice shortform alternativeThere 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.
Might be nice to use something along the lines of
response_deliminator.join(response_list)
here, as it is more readable and allows us to define a response deliminatorThere 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.
We did consider this. Whilst it's conceivable that a response where the individual messages are separated in some specific format could be required, we can't really think of one; so perhaps just add this feature if/when it's needed? We also would have to ensure that both delimiters are passed in as the same type - how much complication could this add?
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 would add any complication if the line were literally
which, I agree with @garryod, is more readable. You could also have
I generally use
functools.reduce
when there aren't alternatives like this.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
response = "".join(response_list)
may introduce typing issues since responses are of typeAnyStr
and we would needb"".join(...)
sometimes?Also, I don't think
sum
works withstr
orbytes
?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.
sum
is literallyfunctools.reduce(lambda a, b: a + b, collection)
, but I think that may be less readable in this case.I think you're right about
join
too, so maybe leave as isThere 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.
Afaik
sum
cannot be used for string types, even withstart="some_str"
. However, the fact thatreduce(lambda a, b: a + b, response_list)
is allowed by mypy is actually erroneous forresponse_list: list[Union[str, bytes]]
as there is no implementation of__add__(self, other)
forself: str
&other: bytes
or vice-versa, as such, we would need to type the input asUnion[list[str], list[bytes]]
or use aTypeVar
to achieve the same (preferable), at which point ensuring both deliminators and the messages are the same type is fairly trivial.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.
@callumforrester not quite sure why it doesn't like strings, but here we are
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 bad...