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

Switch slack library from slacker-asyncio to aioslacker #597

Merged
merged 5 commits into from
Aug 14, 2018

Conversation

jacobtomlinson
Copy link
Member

@jacobtomlinson jacobtomlinson commented Jul 31, 2018

Given all the problems we've had with slacker-asyncio I thought I'd try out aioslacker instead.

The main benefits are:

  • Maintained by the aio-libs org
  • Doesn't override the synchronous slacker module
  • Imports slacker and overrides methods which should be more future compatible
  • Has more flexible dependency specifications

Main drawbacks are:

  • Looks like is hasn't been maintained as recently
  • Currently uses an older version of slacker

Would be interested in thoughts from @FabioRosado and @Cadair

@FabioRosado
Copy link
Member

To be fair our slack connector just needs to get and deliver messages from a slack channel (since we aren't supporting other types of message yet) so I think the fact that aioslacker lib is using an older version os slacker might not pose too much of an issue.

I also haven't received any answer on the issue that I created from the creator of slacker-asyncio so I'm not sure if this lib is actually being maintained actively.

For what you said about the benefits of aioslacker seems like a good way for us to mitigate the issue with the dependency hell issue and if @Cadair had problems with slacker-asyncio overwriting slacker I'm sure someone else will have the same issue in the future.

Basically, this is a big text to say that I might be a good idea to change from slacker-asyncio to aioslacker 👍

@codecov
Copy link

codecov bot commented Aug 4, 2018

Codecov Report

Merging #597 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #597   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          20     20           
  Lines        1369   1369           
=====================================
  Hits         1369   1369
Impacted Files Coverage Δ
opsdroid/connector/slack/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8160a8a...850f374. Read the comment docs.

@FabioRosado
Copy link
Member

Should we merge this and see how everything works or would you like me to give some quick testing on my end?
Also sorry been a bit busy with visiting family and friends - plus trying to survive the 45 degrees @.@

@jacobtomlinson
Copy link
Member Author

I want to do a bit of manual testing. Feel free to do some too.

Don't worry, I just try and do things when I can.

@Cadair
Copy link
Contributor

Cadair commented Aug 14, 2018

👍 This looks like it would solve my issue with the namespace overwriting. Other than that I don't use opsdroid with slack apart from one project (and then I am using slacker directly) so I can't be much help testing. :)

@jacobtomlinson jacobtomlinson merged commit 9c02f9c into opsdroid:master Aug 14, 2018
@jacobtomlinson jacobtomlinson deleted the switch-to-aioslacker branch August 14, 2018 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants