Add more pre-commit hooks and run them in CI

.pre-commit-config.yaml

@@ -1,10 +1,27 @@
+exclude: ^\.cache/|\.venv/|\.git/|htmlcov/|logs/
 repos:
--   repo: local
+  - repo: https://github.com/pre-commit/pre-commit-hooks
+    rev: v2.5.0
     hooks:
-    -   id: flake8
+      - id: check-merge-conflict
+      - id: check-toml
+      - id: check-yaml
+        args: [--unsafe] # Required due to custom constructors (e.g. !ENV)
+      - id: end-of-file-fixer
+      - id: mixed-line-ending
+        args: [--fix=lf]
+      - id: trailing-whitespace
+        args: [--markdown-linebreak-ext=md]
+  - repo: https://github.com/pre-commit/pygrep-hooks
+    rev: v1.5.1
+    hooks:
+      - id: python-check-blanket-noqa
+  - repo: local
+    hooks:
+      - id: flake8
         name: Flake8
         description: This hook runs flake8 within our project's pipenv environment.
-        entry: pipenv run lint
+        entry: pipenv run flake8
         language: python
         types: [python]
-        require_serial: true
+        require_serial: true

CONTRIBUTING.md

@@ -43,7 +43,7 @@ To provide a standalone development environment for this project, docker compose
 When pulling down changes from GitHub, remember to sync your environment using `pipenv sync --dev` to ensure you're using the most up-to-date versions the project's dependencies.
 
 ### Type Hinting
-[PEP 484](https://www.python.org/dev/peps/pep-0484/) formally specifies type hints for Python functions, added to the Python Standard Library in version 3.5. Type hints are recognized by most modern code editing tools and provide useful insight into both the input and output types of a function, preventing the user from having to go through the codebase to determine these types. 
+[PEP 484](https://www.python.org/dev/peps/pep-0484/) formally specifies type hints for Python functions, added to the Python Standard Library in version 3.5. Type hints are recognized by most modern code editing tools and provide useful insight into both the input and output types of a function, preventing the user from having to go through the codebase to determine these types.
 
 For example:
 

Pipfile

@@ -31,6 +31,7 @@ flake8-import-order = "~=0.18"
 flake8-string-format = "~=0.2"
 flake8-tidy-imports = "~=4.0"
 flake8-todo = "~=0.7"
+pep8-naming = "~=0.9"
 pre-commit = "~=2.1"
 safety = "~=1.8"
 unittest-xml-reporting = "~=3.0"
@@ -41,7 +42,7 @@ python_version = "3.8"
 
 [scripts]
 start = "python -m bot"
-lint = "python -m flake8"
+lint = "pre-commit run --all-files"
 precommit = "pre-commit install"
 build = "docker build -t pythondiscord/bot:latest -f Dockerfile ."
 push = "docker push pythondiscord/bot:latest"

azure-pipelines.yml

@@ -13,10 +13,12 @@ jobs:
 
     variables:
       PIP_CACHE_DIR: ".cache/pip"
+      PRE_COMMIT_HOME: $(Pipeline.Workspace)/pre-commit-cache
 
     steps:
       - task: UsePythonVersion@0
         displayName: 'Set Python version'
+        name: PythonVersion
         inputs:
           versionSpec: '3.8.x'
           addToPath: true
@@ -27,8 +29,26 @@ jobs:
       - script: pipenv install --dev --deploy --system
         displayName: 'Install project using pipenv'
 
-      - script: python -m flake8
-        displayName: 'Run linter'
+      # Create an executable shell script which replaces the original pipenv binary.
+      # The shell script ignores the first argument and executes the rest of the args as a command.
+      # It makes the `pipenv run flake8` command in the pre-commit hook work by circumventing
+      # pipenv entirely, which is too dumb to know it should use the system interpreter rather than
+      # creating a new venv.
+      - script: |
+          printf '%s\n%s' '#!/bin/bash' '"${@:2}"' > $(PythonVersion.pythonLocation)/bin/pipenv \
+          && chmod +x $(PythonVersion.pythonLocation)/bin/pipenv
+        displayName: 'Mock pipenv binary'
+
+      - task: Cache@2
+        displayName: 'Restore pre-commit environment'
+        inputs:
+          key: pre-commit | "$(PythonVersion.pythonLocation)" | .pre-commit-config.yaml
+          restoreKeys: |
+            pre-commit | "$(PythonVersion.pythonLocation)"
+          path: $(PRE_COMMIT_HOME)
+
+      - script: pre-commit run --all-files --show-diff-on-failure
+        displayName: 'Run pre-commit hooks'
 
       - script: BOT_API_KEY=foo BOT_SENTRY_DSN=blah BOT_TOKEN=bar WOLFRAM_API_KEY=baz REDDIT_CLIENT_ID=spam REDDIT_SECRET=ham coverage run -m xmlrunner
         displayName: Run tests

bot/cogs/free.py

@@ -55,7 +55,7 @@ async def free(self, ctx: Context, user: Member = None, seek: int = 2) -> None:
                 msg = messages[seek - 1]
             # Otherwise get last message
             else:
-                msg = await channel.history(limit=1).next()  # noqa (False positive)
+                msg = await channel.history(limit=1).next()  # noqa: B305
 
             inactive = (datetime.utcnow() - msg.created_at).seconds
             if inactive > TIMEOUT:

bot/utils/__init__.py

@@ -1,5 +1,4 @@
 from abc import ABCMeta
-from typing import Any, Hashable
 
 from discord.ext.commands import CogMeta
 
@@ -8,59 +7,3 @@ class CogABCMeta(CogMeta, ABCMeta):
     """Metaclass for ABCs meant to be implemented as Cogs."""
 
     pass
-
-
-class CaseInsensitiveDict(dict):
-    """
-    We found this class on StackOverflow. Thanks to m000 for writing it!
-
-    https://stackoverflow.com/a/32888599/4022104
-    """
-
-    @classmethod
-    def _k(cls, key: Hashable) -> Hashable:
-        """Return lowered key if a string-like is passed, otherwise pass key straight through."""
-        return key.lower() if isinstance(key, str) else key
-
-    def __init__(self, *args, **kwargs):
-        super(CaseInsensitiveDict, self).__init__(*args, **kwargs)
-        self._convert_keys()
-
-    def __getitem__(self, key: Hashable) -> Any:
-        """Case insensitive __setitem__."""
-        return super(CaseInsensitiveDict, self).__getitem__(self.__class__._k(key))
-
-    def __setitem__(self, key: Hashable, value: Any):
-        """Case insensitive __setitem__."""
-        super(CaseInsensitiveDict, self).__setitem__(self.__class__._k(key), value)
-
-    def __delitem__(self, key: Hashable) -> Any:
-        """Case insensitive __delitem__."""
-        return super(CaseInsensitiveDict, self).__delitem__(self.__class__._k(key))
-
-    def __contains__(self, key: Hashable) -> bool:
-        """Case insensitive __contains__."""
-        return super(CaseInsensitiveDict, self).__contains__(self.__class__._k(key))
-
-    def pop(self, key: Hashable, *args, **kwargs) -> Any:
-        """Case insensitive pop."""
-        return super(CaseInsensitiveDict, self).pop(self.__class__._k(key), *args, **kwargs)
-
-    def get(self, key: Hashable, *args, **kwargs) -> Any:
-        """Case insensitive get."""
-        return super(CaseInsensitiveDict, self).get(self.__class__._k(key), *args, **kwargs)
-
-    def setdefault(self, key: Hashable, *args, **kwargs) -> Any:
-        """Case insensitive setdefault."""
-        return super(CaseInsensitiveDict, self).setdefault(self.__class__._k(key), *args, **kwargs)
-
-    def update(self, E: Any = None, **F) -> None:
-        """Case insensitive update."""
-        super(CaseInsensitiveDict, self).update(self.__class__(E))
-        super(CaseInsensitiveDict, self).update(self.__class__(**F))
-
-    def _convert_keys(self) -> None:
-        """Helper method to lowercase all existing string-like keys."""
-        for k in list(self.keys()):
-            v = super(CaseInsensitiveDict, self).pop(k)
-            self.__setitem__(k, v)

tests/README.md

@@ -83,7 +83,7 @@ TagContentConverter should return correct values for valid input.
 As we are trying to test our "units" of code independently, we want to make sure that we do not rely objects and data generated by "external" code. If we we did, then we wouldn't know if the failure we're observing was caused by the code we are actually trying to test or something external to it.
 
 
-However, the features that we are trying to test often depend on those objects generated by external pieces of code. It would be difficult to test a bot command without having access to a `Context` instance. Fortunately, there's a solution for that: we use fake objects that act like the true object. We call these fake objects "mocks". 
+However, the features that we are trying to test often depend on those objects generated by external pieces of code. It would be difficult to test a bot command without having access to a `Context` instance. Fortunately, there's a solution for that: we use fake objects that act like the true object. We call these fake objects "mocks".
 
 To create these mock object, we mainly use the [`unittest.mock`](https://docs.python.org/3/library/unittest.mock.html) module. In addition, we have also defined a couple of specialized mock objects that mock specific `discord.py` types (see the section on the below.).
 
@@ -114,13 +114,13 @@ class BotCogTests(unittest.TestCase):
 
 ### Mocking coroutines
 
-By default, the `unittest.mock.Mock` and `unittest.mock.MagicMock` classes cannot mock coroutines, since the `__call__` method they provide is synchronous. In anticipation of the `AsyncMock` that will be [introduced in Python 3.8](https://docs.python.org/3.9/whatsnew/3.8.html#unittest), we have added an `AsyncMock` helper to [`helpers.py`](/tests/helpers.py). Do note that this drop-in replacement only implements an asynchronous `__call__` method, not the additional assertions that will come with the new `AsyncMock` type in Python 3.8. 
+By default, the `unittest.mock.Mock` and `unittest.mock.MagicMock` classes cannot mock coroutines, since the `__call__` method they provide is synchronous. In anticipation of the `AsyncMock` that will be [introduced in Python 3.8](https://docs.python.org/3.9/whatsnew/3.8.html#unittest), we have added an `AsyncMock` helper to [`helpers.py`](/tests/helpers.py). Do note that this drop-in replacement only implements an asynchronous `__call__` method, not the additional assertions that will come with the new `AsyncMock` type in Python 3.8.
 
 ### Special mocks for some `discord.py` types
 
 To quote Ned Batchelder, Mock objects are "automatic chameleons". This means that they will happily allow the access to any attribute or method and provide a mocked value in return. One downside to this is that if the code you are testing gets the name of the attribute wrong, your mock object will not complain and the test may still pass.
 
-In order to avoid that, we have defined a number of Mock types in [`helpers.py`](/tests/helpers.py) that follow the specifications of the actual Discord types they are mocking. This means that trying to access an attribute or method on a mocked object that does not exist on the equivalent `discord.py` object will result in an `AttributeError`. In addition, these mocks have some sensible defaults and **pass `isinstance` checks for the types they are mocking**. 
+In order to avoid that, we have defined a number of Mock types in [`helpers.py`](/tests/helpers.py) that follow the specifications of the actual Discord types they are mocking. This means that trying to access an attribute or method on a mocked object that does not exist on the equivalent `discord.py` object will result in an `AttributeError`. In addition, these mocks have some sensible defaults and **pass `isinstance` checks for the types they are mocking**.
 
 These special mocks are added when they are needed, so if you think it would be sensible to add another one, feel free to propose one in your PR.
 
@@ -144,7 +144,7 @@ Finally, there are some considerations to make when writing tests, both for writ
 
 ### Test coverage is a starting point
 
-Having test coverage is a good starting point for unit testing: If a part of your code was not covered by a test, we know that we have not tested it properly. The reverse is unfortunately not true: Even if the code we are testing has 100% branch coverage, it does not mean it's fully tested or guaranteed to work. 
+Having test coverage is a good starting point for unit testing: If a part of your code was not covered by a test, we know that we have not tested it properly. The reverse is unfortunately not true: Even if the code we are testing has 100% branch coverage, it does not mean it's fully tested or guaranteed to work.
 
 One problem is that 100% branch coverage may be misleading if we haven't tested our code against all the realistic input it may get in production. For instance, take a look at the following `member_information` function and the test we've written for it:
 
@@ -169,7 +169,7 @@ class FunctionsTests(unittest.TestCase):
 
 If you were to run this test, not only would the function pass the test, `coverage.py` will also tell us that the test provides 100% branch coverage for the function. Can you spot the bug the test suite did not catch?
 
-The problem here is that we have only tested our function with a member object that had `None` for the `member.joined` attribute. This means that `member.joined.stfptime("%d-%m-%Y")` was never executed during our test, leading to us missing the spelling mistake in `stfptime` (it should be `strftime`). 
+The problem here is that we have only tested our function with a member object that had `None` for the `member.joined` attribute. This means that `member.joined.stfptime("%d-%m-%Y")` was never executed during our test, leading to us missing the spelling mistake in `stfptime` (it should be `strftime`).
 
 Adding another test would not increase the test coverage we have, but it does ensure that we'll notice that this function can fail with realistic data:
 

tests/base.py

@@ -31,7 +31,7 @@ class LoggingTestsMixin:
     """
 
     @contextmanager
-    def assertNotLogs(self, logger=None, level=None, msg=None):
+    def assertNotLogs(self, logger=None, level=None, msg=None):  # noqa: N802
         """
         Asserts that no logs of `level` and higher were emitted by `logger`.
 
@@ -81,7 +81,7 @@ def assertNotLogs(self, logger=None, level=None, msg=None):
 class CommandTestCase(unittest.IsolatedAsyncioTestCase):
     """TestCase with additional assertions that are useful for testing Discord commands."""
 
-    async def assertHasPermissionsCheck(
+    async def assertHasPermissionsCheck(  # noqa: N802
         self,
         cmd: commands.Command,
         permissions: Dict[str, bool],

tests/bot/cogs/sync/test_base.py

@@ -84,7 +84,7 @@ async def test_send_prompt_gets_dev_core_channel(self):
 
                 method.assert_called_once_with(constants.Channels.dev_core)
 
-    async def test_send_prompt_returns_None_if_channel_fetch_fails(self):
+    async def test_send_prompt_returns_none_if_channel_fetch_fails(self):
         """None should be returned if there's an HTTPException when fetching the channel."""
         self.bot.get_channel.return_value = None
         self.bot.fetch_channel.side_effect = discord.HTTPException(mock.MagicMock(), "test error!")

tests/bot/cogs/test_snekbox.py

@@ -89,15 +89,15 @@ def test_get_results_message(self):
                 self.assertEqual(actual, expected)
 
     @patch('bot.cogs.snekbox.Signals', side_effect=ValueError)
-    def test_get_results_message_invalid_signal(self, mock_Signals: Mock):
+    def test_get_results_message_invalid_signal(self, mock_signals: Mock):
         self.assertEqual(
             self.cog.get_results_message({'stdout': '', 'returncode': 127}),
             ('Your eval job has completed with return code 127', '')
         )
 
     @patch('bot.cogs.snekbox.Signals')
-    def test_get_results_message_valid_signal(self, mock_Signals: Mock):
-        mock_Signals.return_value.name = 'SIGTEST'
+    def test_get_results_message_valid_signal(self, mock_signals: Mock):
+        mock_signals.return_value.name = 'SIGTEST'
         self.assertEqual(
             self.cog.get_results_message({'stdout': '', 'returncode': 127}),
             ('Your eval job has completed with return code 127 (SIGTEST)', '')