Merge pull request #208 from randsleadershipslack/h3h-fixes-and-tests

Improve linting & tests

.config/pycodestyle

@@ -0,0 +1,2 @@
+[pycodestyle]
+max_line_length = 120

.gitignore

@@ -58,3 +58,6 @@ target/
 
 # Where to store your personal API token
 *_token.txt
+
+# VS Code configs
+.vscode/

.travis.yml

@@ -6,7 +6,6 @@ services:
 language: python
 
 python:
-  - 2.7
   - 3.6
 
 cache: pip

bin/install

@@ -1,5 +1,16 @@
 #!/bin/sh
 set -eux
 
-pip install -r build-requirements.txt
-pip install -r requirements.txt
+if [ ${TRAVIS:-false} = 'true' ]; then
+  pip install -r build-requirements.txt
+  pip install -r requirements.txt
+else
+  if command -v pip3 &> /dev/null
+  then
+    pip3 install -r build-requirements.txt
+    pip3 install -r requirements.txt
+  else
+    pip install -r build-requirements.txt
+    pip install -r requirements.txt
+  fi
+fi

bin/test

@@ -3,10 +3,8 @@ set -eux
 
 flake8 --ignore=E501
 
-# Skip tests/test_destalinator.py due to heavy mocking. See https://github.com/jendrikseipp/vulture/issues/95
-# Skip utils/slack_logging.py due to implementing Handler#emit but never calling directly
-# Skip scheduler.py due to unused lambda entrypoint handler
-vulture . --exclude=tests/test_destalinator.py,utils/slack_logging.py,scheduler.py
+# Skip uncalled lambda entry point
+vulture . --ignore-names=destalinate_lambda
 
 coverage run --branch --source=. -m unittest discover -f
 coverage report -m --skip-covered --fail-under=60

build-requirements.txt

@@ -1,6 +1,5 @@
-mock==2.0.0
-six==1.10.0
-coverage==4.4.1
-coveralls==1.2.0
-flake8==3.4.1
-vulture==0.26
+coverage>=5.4
+coveralls>=3.0.0
+flake8>=3.8.4
+mock>=4.0.3
+vulture>=2.3

destalinator.py

@@ -21,9 +21,10 @@ class Destalinator(WithLogger, WithConfig):
 
     def __init__(self, slacker, slackbot, activated):
         """
-        slacker is a Slacker() object
-        slackbot should be an initialized slackbot.Slackbot() object
-        activated is a boolean indicating whether destalinator should do dry runs or real runs
+        `slacker` is a Slacker() object
+        `slackbot` should be an initialized slackbot.Slackbot() object
+        `activated` is a boolean indicating whether destalinator should do dry
+            runs or real runs
         """
         self.closure_text = utils.get_local_file_content(self.closure_text_fname)
         self.warning_text = utils.get_local_file_content(self.warning_text_fname)
@@ -67,8 +68,7 @@ def flush_channel_cache(self, channel_name):
 
     def get_earliest_archive_date(self):
         """Return a datetime.date object representing the earliest archive date."""
-        date_string = self.config.earliest_archive_date \
-            or PAST_DATE_STRING
+        date_string = self.config.earliest_archive_date or PAST_DATE_STRING
         return datetime.strptime(date_string, "%Y-%m-%d").date()
 
     def get_messages(self, channel_name, days):
@@ -77,14 +77,17 @@ def get_messages(self, channel_name, days):
         cid = self.slacker.get_channelid(channel_name)
 
         if oldest in self.cache.get(cid, {}):
-            self.logger.debug("Returning %s cached messages for #%s over %s days", len(self.cache[cid][oldest]), channel_name, days)
+            self.logger.debug("Returning %s cached messages for #%s over %s days", len(
+                self.cache[cid][oldest]), channel_name, days)
             return self.cache[cid][oldest]
 
         messages = self.slacker.get_messages_in_time_range(oldest, cid)
         self.logger.debug("Fetched %s messages for #%s over %s days", len(messages), channel_name, days)
 
-        messages = [x for x in messages if x.get("subtype") is None or x.get("subtype") in self.config.included_subtypes]
-        self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len(messages), ", ".join(self.config.included_subtypes))
+        messages = [x for x in messages if x.get("subtype") is None or x.get(
+            "subtype") in self.config.included_subtypes]
+        self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len(
+            messages), ", ".join(self.config.included_subtypes))
 
         if cid not in self.cache:
             self.cache[cid] = {}
@@ -107,7 +110,8 @@ def post_marked_up_message(self, channel_name, message, **kwargs):
     def stale(self, channel_name, days):
         """
         Return True if channel represented by `channel_name` is stale.
-        Definition of stale is: no messages in the last `days` which are not from config.ignore_users.
+        Definition of stale is: no messages in the last `days` which are not
+        from config.ignore_users.
         """
         if not self.channel_minimum_age(channel_name, days):
             return False
@@ -158,7 +162,8 @@ def archive(self, channel_name):
                 self.logger.info("Archived %s", channel_name)
             else:
                 error = payload.get('error', '!! No error found in payload %s !!' % payload)
-                self.logger.error("Failed to archive %s: %s. See https://api.slack.com/methods/conversations.archive for more context.", channel_name, error)
+                url = "https://api.slack.com/methods/conversations.archive"
+                self.logger.error("Failed to archive %s: %s. See " + url + " for more context.", channel_name, error)
 
             return payload
 
@@ -195,21 +200,13 @@ def warn(self, channel_name, days, force_warn=False):
         Using `force_warn=True` will warn even if a previous warning exists.
         Return True if we actually warned, otherwise False.
         """
-        # Might not need to do this since we now do this in `stale`
-        if self.slacker.channel_has_only_restricted_members(channel_name):
-            self.logger.debug("Would have warned #%s but it contains only restricted users", channel_name)
-            return False
-
-        # Might not need to do this since we now do this in `stale`
-        if self.ignore_channel(channel_name):
-            self.logger.debug("Not warning #%s because it's in ignore_channels", channel_name)
-            return False
 
         messages = self.get_messages(channel_name, days)
         texts = [x.get("text").strip() for x in messages if x.get("text")]
-        if (not force_warn and
-                (self.add_slack_channel_markup(self.warning_text) in texts or
-                 any(any(a.get('fallback') == 'channel_warning' for a in m.get('attachments', [])) for m in messages))):
+        warned_in_message = self.add_slack_channel_markup(self.warning_text) in texts
+        warned_in_attachment = any(any(a.get('fallback') == 'channel_warning' for a in m.get(
+            'attachments', [])) for m in messages)
+        if (not force_warn and (warned_in_message or warned_in_attachment)):
             self.logger.debug("Not warning #%s because we found a prior warning", channel_name)
             return False
 
@@ -228,21 +225,16 @@ def warn_all(self, days, force_warn=False):
 
         stale = []
         for channel in sorted(self.slacker.channels_by_name.keys()):
-            if self.ignore_channel(channel):
-                self.logger.debug("Not warning #%s because it's in ignore_channels", channel)
-                continue
             if self.stale(channel, days):
                 if self.warn(channel, days, force_warn):
                     stale.append(channel)
             self.flush_channel_cache(channel)
 
-        if stale and self.config.general_message_channel:
+        if len(stale) > 0 and self.config.general_message_channel:
             self.logger.debug("Notifying #%s of warned channels", self.config.general_message_channel)
             self.warn_in_general(stale)
 
     def warn_in_general(self, stale_channels):
-        if not stale_channels:
-            return
         if len(stale_channels) > 1:
             channel = "channels"
             being = "are"

flagger.py

@@ -26,7 +26,7 @@ class Flagger(executor.Executor):
 
     def __init__(self, *args, **kwargs):
         self.debug = kwargs.pop('debug', False)
-        super(self.__class__, self).__init__(*args, **kwargs)
+        super(Flagger, self).__init__(*args, **kwargs)
         self.now = int(time.time())
 
     def extract_threshold(self, token):
@@ -40,8 +40,8 @@ def extract_threshold(self, token):
         <int
         returns [comparator, int] or throws error if invalid
         """
-        comparator = re.sub("\d+$", "", token)
-        value = int(re.sub("\D*", "", token))
+        comparator = re.sub(r"\d+$", "", token)
+        value = int(re.sub(r"\D*", "", token))
         if comparator == '':  # no comparator specified
             comparator = '>='
 
@@ -83,7 +83,7 @@ def initialize_control(self):
                 emoji = tokens[5].replace(":", "")
                 output_channel_id = re.sub("[<>]", "", tokens[6])
                 if output_channel_id.find("|") != -1:
-                    cid, cname = output_channel_id.split("|")
+                    cid, _ = output_channel_id.split("|")
                     output_channel_id = cid
                 output_channel_name = self.slacker.replace_id(output_channel_id)
                 control[uuid] = {'threshold': threshold, "comparator": comparator,
@@ -209,7 +209,7 @@ def announce_interesting_messages(self):
                     if not self.debug and self.config.activated:  # TODO: rename debug to dry run?
                         self.slackbot.say(output_channel["output"], m)
                 else:
-                    self.logger.warning("Attempted to announce in %s because of rule :%s:%s%s, but channel does not exist.".format(
+                    self.logger.warning("Attempted to announce in {} because of rule :{}:{}{}, but channel does not exist.".format(
                         output_channel["output"],
                         output_channel["emoji"],
                         output_channel["comparator"],

requirements.txt

@@ -1,4 +1,4 @@
 APScheduler>=3.0.5
 raven>=6.1.0
 requests>=2.20.0
-ruamel.yaml
+ruamel.yaml>=0.6.12

scheduler.py

@@ -22,7 +22,7 @@ def schedule_job():
     sched.start()
 
 
-def destalinate_lambda(event, context):
+def destalinate_lambda(_event, _context):
     destalinate_job()
 
 
@@ -31,10 +31,8 @@ def destalinate_job():
 
     logging.info("Destalinating")
     if not get_config().sb_token or not get_config().api_token:
-        logging.error(
-            "Missing at least one required Slack environment variable.\n"
-            "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN."
-        )
+        logging.error("Missing at least one required Slack environment variable.\n"
+                      "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN.")
     else:
         try:
             archiver.Archiver().archive()