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

Goonj code refactor #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

AdityaMisra
Copy link

@AdityaMisra AdityaMisra commented Jun 6, 2018

I have just refactored the code.

  1. I have moved threading logic to AsyncProcess class
  2. Moved send_message() method to base class and changes it to an abstract method
  3. Moved the Singleton logic to a metaclass
  4. updated requirements.txt
  5. Moved some duplicate code to a single place.

I know I'm not suppose to merge a PR without anybody's review. I was waiting for Bibek to review it. I've sent him the PR.

If you get time can you review it.

Aditya Misra and others added 2 commits May 31, 2018 15:56
@varunachar
Copy link
Contributor

@AdityaMisra What is this pull request for? Please add details.

Second, you are not allowed to merge a pull request without review.

@varunachar
Copy link
Contributor

Thanks for adding the details.

I've just finished reviewing Sohit's PR.. Can you two sync up and see who needs to back merge from whom? He's adding functionality and you're refactoring, so best keep things in sync. I can then do the PR.

@keshrisohit

@AdityaMisra
Copy link
Author

@varunachar I'll sync up with @keshrisohit


class BaseChannel(object):
"""
Base class for all alerting channels
"""

@abc.abstractmethod
def send(self, sev, message, subject, error_id, error, tag_list):
def send_message(self, sev, message, subject=None, error_id=None, error=None, tag_list=None):
Copy link

Choose a reason for hiding this comment

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

make this private by adding _ so that noone will use it directly

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

Successfully merging this pull request may close these issues.

3 participants