Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Add attachments to extensions #5

Merged
merged 4 commits into from
Jan 26, 2018
Merged

Conversation

ridwanmsharif
Copy link

Cherrypicked and amended styling for commits made for PR litaio#118 in lita-slack that is currently unmaintained. Allowing for attachments being made available in the extensions, would help us address issues we're having with Spy. Review required to see if the changes are appropriate. @Shopify/operational-excellence

Copy link

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

Hella minor nitpick you don't have to fix. The word attachment is in the method name and it's short AF. Mostly pointing out that, in general, one-letter parameters/arguments/variables aren't great.
Works here though.

🚢


([normalized_message] + attachment_text).compact.join("\n")
def parse_attachment(a)

Choose a reason for hiding this comment

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

⛏ In a perfect world, this would be attachement rather than just a.

Copy link
Author

Choose a reason for hiding this comment

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

It'll bug me now if I don't fix it - give me a sec

@@ -118,7 +130,7 @@ def dispatch_message(user)
source.private_message! if channel && channel[0] == "D"
message = Message.new(robot, body, source)
message.command! if source.private_message?
message.extensions[:slack] = { timestamp: data["ts"] }
message.extensions[:slack] = { timestamp: data["ts"], attachments: data["attachments"] }

Choose a reason for hiding this comment

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

💖

@ridwanmsharif ridwanmsharif force-pushed the add-attachments-to-extensions branch from 39aa1b6 to b0ffefc Compare January 25, 2018 20:28
Copy link

@jarthorn jarthorn left a comment

Choose a reason for hiding this comment

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

I'm not going to give a strongly opinionated review because it is someone else's change.. but +1 to merge this and if we find issues we can apply them on top of it.

Copy link

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

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

Shout out to @NickolasVashchenko, thanks for this <3

@NikolayRys
Copy link

Sure, guys :)

@ridwanmsharif ridwanmsharif merged commit 84b522e into master Jan 26, 2018
@RyanBrushett RyanBrushett deleted the add-attachments-to-extensions branch January 26, 2018 02:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants