Skip to content
This repository has been archived by the owner on Jun 30, 2022. It is now read-only.

Ability to send content as a file. #86

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

boone
Copy link

@boone boone commented Mar 4, 2016

We often need to attach text dumps to our bot replies. This PR adds a send_file_content method to handle this. It uses the content argument of Slack's files.upload API call, which the docs suggest is the proper way to create a file from a "long message/paste".

This is my first Lita-related PR so feedback is appreciated.

@boone
Copy link
Author

boone commented Mar 4, 2016

Not sure what's up with the Travis failure. The specs pass locally, and they are based on the specs for set_topic which work OK.

@jimmycuadra
Copy link
Contributor

Thanks for the PR and welcome to the Lita community! This is very useful functionality and I'd like to get it or something like it added to the adapter. I won't be able to merge this as is, though, for a few reasons:

  • A general abstraction for file uploads in Lita has not yet landed (see Upload file abstraction lita#89). Assuming something like that will eventually be in Lita, I'd rather have the Slack adapter use that API and expose it directly on the adapter.
  • A prerequisite to a good upload API is to be able to stream the contents of the file so that the entire thing doesn't need to be loaded into memory at once. Unfortunately, Faraday, the HTTP library that both Lita and lita-slack use, does not support streaming. It's one of my goals to eventually replace Faraday with the http gem, which does support streaming, but this hasn't happened yet.

I'll leave this PR open to track the desire for this feature, but I'm afraid it'll need to wait for the above points to be addressed first.

P.S. You can rebase onto the latest master and you'll see the spec failure locally—your commit is based on an older master.

@pdaugavietis
Copy link

Is this going to be merged soon - it's going to be a great problem solver from a chatops point of view, and making certain functionalites easily accessible by our users.

@jimmycuadra
Copy link
Contributor

Nope, my previous comment is still the current situation.

nwesoccer added a commit to PowerDMS/lita-slack that referenced this pull request Oct 20, 2017
@joseluis-fw
Copy link

@jimmycuadra what's the status to merge this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants