Update new methods #326
Update new methods #326
Conversation
|
@dblock, not entirely sure if I've approached the patching (dd6b484) the right way, appreciate feedback. It was a bit painful to patch
Seems like there aren't that many method patches, so hopefully I won't need to repeat this many times when we add the deprecation message. There's also a bunch of new methods / commands that got added in 80a3962, I wasn't sure if you wanted to add them, it's an easy revert anyway. |
dblock
left a comment
dblock
left a comment
There was a problem hiding this comment.
Something's wrong with the generated patches and the + stuff.
Let's split up this PR into manageable things to review, maybe start by taking out the reconnected business?
The CONTRIBUTING doc could use a better getting started for the patches. Since you've just done it, maybe update it too?
lib/slack/real_time/client.rb
Outdated
| end | ||
|
|
||
| def reconnected? | ||
| @reconnected ||= false |
There was a problem hiding this comment.
This seems unrelated, but regardless you don't want to have a side-effect on calling a property method of assigning anything. So this could be !! @reconnected to coerce it into a boolean, for example.
There was a problem hiding this comment.
I think I might have left this by accident while looking at slack-ruby/slack-ruby-bot#244 >_<
| attachments = JSON.dump(attachments) unless attachments.is_a?(String) | ||
| options = options.merge(attachments: attachments) | ||
| end | ||
| + # attachments must be passed as an encoded JSON string |
There was a problem hiding this comment.
Those + signs look like a patch bug, this code won't run :)
There was a problem hiding this comment.
Possibly missing something here, but isn't this the output of the diff? I was able to run the rake task fine, so I don't think it causes an issue with the patching. Example output from the older files also has the + signs.
There was a problem hiding this comment.
Bah, I don't know what I was thinking, this is a patch file. You're right.
So what happened here? Before we were removing the code that did JSON.dump and now we put it back? Seems like the new(er) version is correct.
There was a problem hiding this comment.
Oh looks like multiple patches got merged into one. I get it, ignore me.
I've kept only the chat changes for now! |
Generated by 🚫 Danger |
| ##### Resolving Patch Errors | ||
| The auto-generated method files may drift overtime e.g. new arguments may be added or descriptions changed. Since previous patches were based on the older auto-generated files, git may be unable to apply them to the new files. Resolving them requires some good ol' splicing: | ||
| 1. Comment out the patching code in `lib/tasks/web.rake`: |
There was a problem hiding this comment.
TODO for later, we should add a rake task option that skips patching, so rake slack:api:update --no-patch or something like that.
|
Merged, 👍 |
3 participants
Prereq to #320.