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

adding slack and pushbullet #9

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

Conversation

bjay1404
Copy link

I added slack and pushbullet

@bjay1404
Copy link
Author

Thoughts on the code?

@Chronial
Copy link
Owner

Hey, sry for not responding sooner – I didn't yet find time to go over the whole PR.

At a quick glance there seem to be some issues with it:

  • How does this handle the fact that existing config files don't have a pushbullet and slack config?
  • The comments in the config file don't seem correct
  • What does the title config in the slack section do?
  • Why did you rename the default config file name?
  • Won't this crash for everybody who doesn't have the slackclient python package installed?

apikey =

[slack]
; set to true to enable Pushbullet notification support
Copy link

Choose a reason for hiding this comment

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

Copy-paste error 😉

from collections import Counter, defaultdict
from cStringIO import StringIO

from slackclient import SlackClient
Copy link

Choose a reason for hiding this comment

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

No need really to add this dependency for everyone - could instead make it optional and import it within send_slack? (cf.send_email)

Choose a reason for hiding this comment

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

Good point, I don't use the slack client with my other code at this point. I use requests to make slack calls

@@ -156,7 +212,7 @@ def setup_logger():
root_logger.addHandler(console_logger)

if config["logging"]["file"]:
max_log_size = min(config["logging"]["maxsize"], 0) * 1024
max_log_size = max(config["logging"]["maxsize"], 0) * 1024
Copy link

Choose a reason for hiding this comment

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

Looks accidental?

@@ -178,7 +234,7 @@ def setup_logger():
def main():
parser = argparse.ArgumentParser()
parser.add_argument("-c", "--conf",
default="snapraid-runner.conf",
default="snapraid-runner ",
Copy link

Choose a reason for hiding this comment

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

Looks accidental?

from collections import Counter, defaultdict
from cStringIO import StringIO

from slackclient import SlackClient

Choose a reason for hiding this comment

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

Good point, I don't use the slack client with my other code at this point. I use requests to make slack calls

@Chronial Chronial added notfication-service out-of-scope This feature is of scope for the (narrow) goals of snapraid-runner labels Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notfication-service out-of-scope This feature is of scope for the (narrow) goals of snapraid-runner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants