Mostly type-hinted helper classes for inclusion in v2 #400
Conversation
|
@paul-griffith This is awesome! I'm still reading through it, but I'm very interested in working together to get this into a mergeable state for v2. |
|
This is super nice! it would help me a lot! |
|
It would be awesome for this to be integrated into |
|
There are currently a few high priority bugs I'm working on so it's taken me a while to get to this. This is definitely still a PR we'd like to see merged! Right now the biggest blocker is Unit tests and documentation. |
RodneyU215
added
Priority: Medium
enhancement
|
The question is if you want to merge this as-is, with the extra wrapper, or if you maybe want to integrate the helper classes with the existing clients. I'd say the latter is preferable, but that requires some extra work which I don't know if @paul-griffith wants to put in. |
|
No, the extra wrapper is an area that I'd like to discuss more. Preferably, I'd love to work to get all of the message builder classes merged first. This could be used immediately with the existing API methods. |
|
@paul-griffith I want to thank you again for contributing such a phenomenal PR. It's clear you've put a great deal of work into this and it's going to be provide a lot of value to the community. |
|
If I can help put this into the desired shape, let me know @paul-griffith @RodneyU215. This addition would greatly simplify the code of my own bot framework |
|
I've made improvements and started to add tests to my own lib since I
opened this, but before I do more specific work for integrating I want to
double check with my own boss about how much time I can dedicate to this. I
also just updated my own codebase to work with v2 of this package.
Rodney, may be worth arranging a time we can have a quick discussion
real-time to see what you would want changed, also - that would help me see
how much time would be needed.
…On Tue, Jun 4, 2019, 10:52 AM Daan Debie ***@***.***> wrote:
If I can help put this into the desired shape, let me know @paul-griffith
<https://github.com/paul-griffith> @RodneyU215
<https://github.com/RodneyU215>. This addition would greatly simplify the
code of my own bot framework <https://github.com/DandyDev/slack-machine>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#400>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AJMFY3UAXMEKRZZZEWTSZGLPY2TVTANCNFSM4HIPHIIQ>
.
|
|
@paul-griffith Happy to chat this week. I've sent you a DM in dev4slack and you can also email me at |
…started adding more tests
|
I don't plan on significantly changing anything in the core lib at this point - just need to add test coverage at this point. Happy for anyone else to contribute tests, or even just known-failing-cases - coverage right now is basically limited to whatever is explicitly not allowed in the API documentation, but I'm confident there's plenty of scenarios that aren't covered by the JSON validation this currently has. |
Codecov Report
@@ Coverage Diff @@
## master #400 +/- ##
===========================================
+ Coverage 56.94% 70.03% +13.08%
===========================================
Files 6 15 +9
Lines 734 1622 +888
Branches 42 91 +49
===========================================
+ Hits 418 1136 +718
- Misses 307 464 +157
- Partials 9 22 +13
Continue to review full report at Codecov.
|
| footer_icon: str = None, | ||
| ts: int = None, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
The docstring/param list on these two classes is a bit ridiculous, but there's not really any meta-information to keep track of that helps make a builder class worthwhile, IMO
There was a problem hiding this comment.
I agree with this position. For now until new data surfaces let's stick with your recommendation.
There was a problem hiding this comment.
On consideration, I could see some utility to an AttachmentBuilder - something like:
AttachmentBuilder()
.text("Some attachment")
.footer("footer", icon="icon url")
.author("jim bob", link="jimbob.com", icon="jimbob.com/picture.jpg")
.field("*Score*", "20")
.field("*Series*", "Some book series")
.linkbutton("button", "Click me!", "http://google.com")
.user_selector()
.callback_id("abcdefg")
.build()
But, that also ties into the point that pure attachments shouldn't really be encouraged - although I do hope either blocks get a color attribute soon or BlockAttachments never go away 🤞
There was a problem hiding this comment.
While I agree an AttachementBuilder would be nice, I would not recommend investing too much time into it. Block Kit is the focus and will continue to see new features added.
| footer_icon: str = None, | ||
| ts: int = None, | ||
| ): | ||
| """ |
There was a problem hiding this comment.
I agree with this position. For now until new data surfaces let's stick with your recommendation.
|
|
||
| def get_json(self, option_type: str = "block") -> dict: | ||
| if option_type == "action": | ||
| # deliberately skipping JSON validators here - can't find documentation |
There was a problem hiding this comment.
Note for myself: Check on limits described here.
|
@dandydev If you could spare a moment to review this PR I'd love your feedback on this as well. |
|
@RodneyU215 I will have a look at it tonight (GMT+2 time). Happy to help! |
There was a problem hiding this comment.
This looks great! So useful!
Some general comments:
- There are some places where you have constructors that accept parameters with default value
None. Technically, a better type annotation for those parameters would beOptional[X]whenXis the type of the value in case it's notNone. This allows MyPy (or for example PyCharm's built-in type checker) to give even better type hints. Although I can also see how that would become a bit cumbersome. - I'm not a big fan of the name
get_json(), because it doesn't strictly return json (string). It returns a dict. I realize that this dict is eventually converted into a json string, but I still feel it can be a confusing name. I must admit that I don't know of a better alternative right now. The same problem also exists with theJsonXXXclasses (to a lesser extent in my opinion)
Done! |
|
Solid points, @dandydev. I moved the dialog sub-elements into a Agreed on Also fixed the use of
|
Great!
I like
I see a couple of element categories that could be split out:
That's the only split I can think of. |
|
One other option with |
|
I'd say, leave it for now. For me it's a minor nitpick. If stuff is added to |
|
Okay, @RodneyU215, I'll defer to you on decisions about:
Otherwise, I think this is almost ready to merge? 🤞 |
|
@dandydev Thanks for the review!
Thank you both for the progress on this. We're very close to getting this merged. Once the rename is complete let's get this shipped. |
RodneyU215
left a comment
RodneyU215
left a comment
There was a problem hiding this comment.
Awesome! This is great. Thank you!
4 participants
Summary
This is a direct copy-paste of the pseudo-library that wraps
python-slackclientI use internally. The big advantage is obviously typed interactions - especially the newer stuff (ie, blockkit) has pretty solid type hinting throughout.I'm offering up this PR without any adjustments having been made - if it's something y'all are interested in, I could potentially spend some time making changes to better fit it into this library. Happy to review with anybody - I'm also in the dev4slack group semi-often if you want to reach me realtime.
Requirements (place an
xin each[ ])