Skip to content
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

Add JSON encoding to all applicable Web API arguments #448

Merged
merged 14 commits into from
Mar 21, 2023

Conversation

jmanian
Copy link
Collaborator

@jmanian jmanian commented Mar 17, 2023

There was at least one chat.* method that was missing the auto-encoding of blocks and attachments into JSON (chat.scheduleMessage). Instead of continuing to apply these to each method individually as a patch, I thought it would be nice to solve this once and for all.

New groups of methods or new options that require encoding will require changes to accommodate those, but no patching. This is now being pulled from data in slack-api-ref and will update automatically with the docs.

I'm open to suggestions on the exact approach, and I'm opening this as a draft without tests, changelog, etc. to gather comments first.

@jmanian jmanian requested review from dblock and kstole March 17, 2023 15:34
Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this very much! See my suggestions/comments below. Definitely needs tests.

lib/slack/web/api/options.rb Outdated Show resolved Hide resolved
lib/slack/web/api/options.rb Outdated Show resolved Hide resolved
lib/slack/web/api/templates/method.erb Outdated Show resolved Hide resolved
@jmanian jmanian requested a review from dblock March 17, 2023 19:24
@jmanian
Copy link
Collaborator Author

jmanian commented Mar 17, 2023

@dblock Here's a different take, trying to base it entirely on data in slack-api-ref. It feels a little fragile to base it on data in the argument descriptions, but there isn't anything more structured at the moment (in slack-api-ref or on the doc pages). It does find all the ones we knew about, plus a handful of others.

Copy link
Collaborator

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot better! I think you should modify slack-api-ref to make it less fragile, aka parse the description for json in its scraper, and generate a field.

@jmanian
Copy link
Collaborator Author

jmanian commented Mar 20, 2023

@dblock still needs tests and changelog, but here's what this looks like now with the slack-ruby/slack-api-ref#64

@dblock
Copy link
Collaborator

dblock commented Mar 20, 2023

Looks good!

@jmanian jmanian marked this pull request as ready for review March 20, 2023 19:04
@@ -1,6 +1,7 @@
### 2.1.1 (Next)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's bump the version to 2.2 and make a release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'm going to put up another PR to replace some of the patched argument validations with the new arg_groups data from the ref, so I'll do it in that.

@dblock dblock merged commit 16ffbf5 into slack-ruby:master Mar 21, 2023
@jmanian jmanian deleted the json-encoding branch March 21, 2023 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants