Merge pull request #222 from rgalonso/refactor-and-add-tests

refactor and add tests

.devcontainer/devcontainer.json

@@ -0,0 +1,22 @@
+// For format details, see https://aka.ms/devcontainer.json. For config options, see the
+// README at: https://github.com/devcontainers/templates/tree/main/src/python
+{
+	"name": "Python 3",
+	// Or use a Dockerfile or Docker Compose file. More info: https://containers.dev/guide/dockerfile
+	"image": "mcr.microsoft.com/devcontainers/python:1-3.12-bullseye",
+
+	// Features to add to the dev container. More info: https://containers.dev/features.
+	// "features": {},
+
+	// Use 'forwardPorts' to make a list of ports inside the container available locally.
+	// "forwardPorts": [],
+
+	// Use 'postCreateCommand' to run commands after the container is created.
+	"postCreateCommand": "pip3 install --user -r requirements.txt -r requirements-dev.txt"
+
+	// Configure tool-specific properties.
+	// "customizations": {},
+
+	// Uncomment to connect as root instead. More info: https://aka.ms/dev-containers-non-root.
+	// "remoteUser": "root"
+}

.github/workflows/ci.yml

@@ -13,6 +13,6 @@ jobs:
       - name: "Install test dependencies"
         run: |
           python -m pip install --upgrade pip
-          pip install -r requirements.txt
+          pip install -r requirements.txt -r requirements-dev.txt
       - name: "Run tests"
         run: python -m pytest

Client.py

@@ -0,0 +1,12 @@
+class Client(object):
+    def get_last_diff(self):
+        return None
+
+    def create_issue(self, issue):
+        return [201, None]
+
+    def close_issue(self, issue):
+        return 200
+
+    def get_issue_url(self, new_issue_number):
+        return "N/A"

Dockerfile

@@ -6,6 +6,13 @@ RUN pip install --target=/app requests
 RUN pip install --target=/app -U pip setuptools wheel
 RUN pip install --target=/app ruamel.yaml
 
+FROM ubuntu AS ubuntu-runtime
+RUN apt update -y && apt install -y python3 git
+COPY --from=builder /app /app
+WORKDIR /app
+ENV PYTHONPATH /app
+CMD ["python3", "/app/main.py"]
+
 FROM gcr.io/distroless/python3-debian12
 COPY --from=builder /app /app
 WORKDIR /app

GitHubClient.py

@@ -1,8 +1,9 @@
 import os
 import requests
 import json
+from Client import Client
 
-class GitHubClient(object):
+class GitHubClient(Client):
     """Basic client for getting the last diff and managing issues."""
     existing_issues = []
     milestones = []
@@ -17,7 +18,7 @@ class GitHubClient(object):
         self.before = os.getenv('INPUT_BEFORE')
         self.sha = os.getenv('INPUT_SHA')
         self.commits = json.loads(os.getenv('INPUT_COMMITS')) or []
-        self.diff_url = os.getenv('INPUT_DIFF_URL')
+        self.__init_diff_url__()
         self.token = os.getenv('INPUT_TOKEN')
         self.issues_url = f'{self.repos_url}{self.repo}/issues'
         self.milestones_url = f'{self.repos_url}{self.repo}/milestones'
@@ -34,7 +35,6 @@ class GitHubClient(object):
         self.line_break = '\n\n' if auto_p else '\n'
         self.auto_assign = os.getenv('INPUT_AUTO_ASSIGN', 'false') == 'true'
         self.actor = os.getenv('INPUT_ACTOR')
-        self.insert_issue_urls = os.getenv('INPUT_INSERT_ISSUE_URLS', 'false') == 'true'
         if self.base_url == 'https://api.github.com/':
             self.line_base_url = 'https://github.com/'
         else:
@@ -45,6 +45,20 @@ class GitHubClient(object):
         # Populate milestones so we can perform a lookup if one is specified.
         self._get_milestones()
 
+    def __init_diff_url__(self):
+        manual_commit_ref = os.getenv('MANUAL_COMMIT_REF')
+        manual_base_ref = os.getenv('MANUAL_BASE_REF')
+        if manual_commit_ref:
+            self.sha = manual_commit_ref
+        if manual_commit_ref and manual_base_ref:
+            print(f'Manually comparing {manual_base_ref}...{manual_commit_ref}')
+            self.diff_url = f'{self.repos_url}{self.repo}/compare/{manual_base_ref}...{manual_commit_ref}'
+        elif manual_commit_ref:
+            print(f'Manual checking {manual_commit_ref}')
+            self.diff_url = f'{self.repos_url}{self.repo}/commits/{manual_commit_ref}'
+        else:
+            self.diff_url = os.getenv('INPUT_DIFF_URL')
+
     def get_last_diff(self):
         """Get the last diff."""
         if self.diff_url:
@@ -56,10 +70,12 @@ class GitHubClient(object):
         elif len(self.commits) == 1:
             # There is only one commit.
             diff_url = f'{self.repos_url}{self.repo}/commits/{self.sha}'
-        else:
+        elif len(self.commits) > 1:
             # There are several commits: compare with the oldest one.
             oldest = sorted(self.commits, key=self._get_timestamp)[0]['id']
             diff_url = f'{self.repos_url}{self.repo}/compare/{oldest}...{self.sha}'
+        else:
+            return None
 
         diff_headers = {
             'Accept': 'application/vnd.github.v3.diff',
@@ -352,4 +368,4 @@ class GitHubClient(object):
         return pr_request.status_code
 
     def get_issue_url(self, new_issue_number):
-        return f'Issue URL: {self.line_base_url}{self.repo}/issues/{new_issue_number}'
+        return f'{self.line_base_url}{self.repo}/issues/{new_issue_number}'

LocalClient.py

@@ -1,16 +1,34 @@
 import subprocess
+import os
+from Client import Client
 
-class LocalClient(object):
+class LocalClient(Client):
     def __init__(self):
         self.diff_url = None
         self.commits = ['placeholder'] # content doesn't matter, just length
         self.insert_issue_urls = False
+        self.__set_diff_refs__()
+
+    def __set_diff_refs__(self):
+        # set the target of the comparison to user-specified value, if
+        # provided, falling back to HEAD
+        manual_commit_ref = os.getenv('MANUAL_COMMIT_REF')
+        if manual_commit_ref:
+            self.sha = manual_commit_ref
+        else:
+            self.sha = subprocess.run(['git', 'rev-parse', 'HEAD'], stdout=subprocess.PIPE).stdout.decode('utf-8').strip()
+        # set the soruce of the comparison to user-specified value, if
+        # provided, falling back to commit immediately before the target
+        manual_base_ref = os.getenv('MANUAL_BASE_REF')
+        if manual_base_ref:
+            self.base_ref = manual_base_ref
+        else:
+            self.base_ref = subprocess.run(['git', 'rev-parse', f'{self.sha}^'], stdout=subprocess.PIPE).stdout.decode('utf-8').strip()
+        # print feedback to the user
+        if manual_commit_ref and manual_base_ref:
+            print(f'Manually comparing {manual_base_ref}...{manual_commit_ref}')
+        elif manual_commit_ref:
+            print(f'Manual checking {manual_commit_ref}')
 
     def get_last_diff(self):
-        return subprocess.run(['git', 'diff', 'HEAD^..HEAD'], stdout=subprocess.PIPE).stdout.decode('utf-8')
-
-    def create_issue(self, issue):
-        return [201, None]
-
-    def close_issue(self, issue):
-        return 200
+        return subprocess.run(['git', 'diff', f'{self.base_ref}..{self.sha}'], stdout=subprocess.PIPE).stdout.decode('latin-1')

main.py

@@ -10,46 +10,37 @@ import operator
 from collections import defaultdict
 from TodoParser import TodoParser
 from LineStatus import LineStatus
+from Client import Client
 from LocalClient import LocalClient
 from GitHubClient import GitHubClient
 
 
 if __name__ == "__main__":
+    client: Client | None = None
     # Try to create a basic client for communicating with the remote version control server, automatically initialised with environment variables.
     try:
         # try to build a GitHub client
         client = GitHubClient()
     except EnvironmentError:
         # don't immediately give up
-        client = None
+        pass
     # if needed, fall back to using a local client for testing
     client = client or LocalClient()
 
-    # Check to see if the workflow has been run manually.
-    # If so, adjust the client SHA and diff URL to use the manually supplied inputs.
-    manual_commit_ref = os.getenv('MANUAL_COMMIT_REF')
-    manual_base_ref = os.getenv('MANUAL_BASE_REF')
-    if manual_commit_ref:
-        client.sha = manual_commit_ref
-    if manual_commit_ref and manual_base_ref:
-        print(f'Manually comparing {manual_base_ref}...{manual_commit_ref}')
-        client.diff_url = f'{client.repos_url}{client.repo}/compare/{manual_base_ref}...{manual_commit_ref}'
-    elif manual_commit_ref:
-        print(f'Manual checking {manual_commit_ref}')
-        client.diff_url = f'{client.repos_url}{client.repo}/commits/{manual_commit_ref}'
-    if client.diff_url or len(client.commits) != 0:
-        # Get the diff from the last pushed commit.
-        last_diff = StringIO(client.get_last_diff())
+    # Get the diff from the last pushed commit.
+    last_diff = client.get_last_diff()
+
+    if last_diff:
         # Parse the diff for TODOs and create an Issue object for each.
-        raw_issues = TodoParser().parse(last_diff)
+        raw_issues = TodoParser().parse(StringIO(last_diff))
         # This is a simple, non-perfect check to filter out any TODOs that have just been moved.
         # It looks for items that appear in the diff as both an addition and deletion.
         # It is based on the assumption that TODOs will not have identical titles in identical files.
         # That is about as good as we can do for TODOs without issue URLs.
         issues_to_process = []
-        for values, similar_issues in itertools.groupby(raw_issues, key=operator.attrgetter('title', 'file_name',
+        for values, similar_issues_iter in itertools.groupby(raw_issues, key=operator.attrgetter('title', 'file_name',
                                                                                             'markdown_language')):
-            similar_issues = list(similar_issues)
+            similar_issues = list(similar_issues_iter)
             if (len(similar_issues) == 2 and all(issue.issue_url is None for issue in similar_issues)
                     and ((similar_issues[0].status == LineStatus.ADDED
                           and similar_issues[1].status == LineStatus.DELETED)
@@ -84,16 +75,18 @@ if __name__ == "__main__":
         issues_to_process = [issue for issue in issues_to_process if
                              not (issue.issue_url in update_and_close_issues and issue.status == LineStatus.DELETED)]
 
+        # Check to see if we should insert the issue URL back into the linked TODO.
+        insert_issue_urls = os.getenv('INPUT_INSERT_ISSUE_URLS', 'false') == 'true'
+
         # Cycle through the Issue objects and create or close a corresponding GitHub issue for each.
         for j, raw_issue in enumerate(issues_to_process):
-            print(f'Processing issue {j + 1} of {len(issues_to_process)}')
+            print(f"Processing issue {j + 1} of {len(issues_to_process)}: '{raw_issue.title}' @ {raw_issue.file_name}:{raw_issue.start_line}")
             if raw_issue.status == LineStatus.ADDED:
                 status_code, new_issue_number = client.create_issue(raw_issue)
                 if status_code == 201:
-                    print('Issue created')
-                    # Check to see if we should insert the issue URL back into the linked TODO.
+                    print(f'Issue created: : #{new_issue_number} @ {client.get_issue_url(new_issue_number)}')
                     # Don't insert URLs for comments. Comments do not get updated.
-                    if client.insert_issue_urls and not (raw_issue.ref and raw_issue.ref.startswith('#')):
+                    if insert_issue_urls and not (raw_issue.ref and raw_issue.ref.startswith('#')):
                         line_number = raw_issue.start_line - 1
                         with open(raw_issue.file_name, 'r') as issue_file:
                             file_lines = issue_file.readlines()
@@ -101,7 +94,7 @@ if __name__ == "__main__":
                             # Duplicate the line to retain the comment syntax.
                             new_line = file_lines[line_number]
                             remove = fr'{raw_issue.identifier}.*{raw_issue.title}'
-                            insert = client.get_issue_url(new_issue_number)
+                            insert = f'Issue URL: {client.get_issue_url(new_issue_number)}'
                             new_line = re.sub(remove, insert, new_line)
                             # Check if the URL line already exists, if so abort.
                             if line_number == len(file_lines) - 1 or file_lines[line_number + 1] != new_line:
@@ -109,7 +102,7 @@ if __name__ == "__main__":
                                 with open(raw_issue.file_name, 'w') as issue_file:
                                     issue_file.writelines(file_lines)
                 elif status_code == 200:
-                    print('Issue updated')
+                    print(f'Issue updated: : #{new_issue_number} @ {client.get_issue_url(new_issue_number)}')
                 else:
                     print('Issue could not be created')
             elif raw_issue.status == LineStatus.DELETED and os.getenv('INPUT_CLOSE_ISSUES', 'true') == 'true':

requirements-dev.txt

@@ -0,0 +1,4 @@
+pytest==8.3.3
+mypy==1.13.0
+mypy-extensions==1.0.0
+types-requests==2.32.0.20241016

requirements.txt

@@ -1,3 +1,2 @@
 requests==2.32.3
-ruamel.yaml==0.18.6
-pytest==8.3.3
+ruamel.yaml==0.18.6

tests/test_syntax.py

@@ -0,0 +1,39 @@
+# based on https://gist.github.com/bbarker/4ddf4a1c58ae8465f3d37b6f2234a421
+
+import os
+import subprocess
+import sys
+import unittest
+from typing import List
+
+
+class MyPyTest(unittest.TestCase):
+
+    def __call_mypy__(self, args, files):
+        result: int = subprocess.call(self.base_mypy_call + args + files, env=os.environ, cwd=self.pypath)
+        self.assertEqual(result, 0, '')
+
+    def test_run_mypy_app(self):
+        mypy_args: List[str] = [
+            "--disable-error-code", "var-annotated"
+        ]
+        self.__call_mypy__(mypy_args, ["main.py"])
+
+    # Run test again, but without disabling any error codes.
+    # This is expected to fail, but we intentionally keep this test around to
+    #   1) try not to add any more errors to what's already in the baseline
+    #   2) as a reminder to try to move the codebase towards having type checking eventually
+    @unittest.expectedFailure
+    def test_run_strict_mypy_app(self):
+        mypy_args: List[str] = []
+        self.__call_mypy__(mypy_args, ["main.py"])
+
+    def __init__(self, *args, **kwargs) -> None:
+        super(MyPyTest, self).__init__(*args, **kwargs)
+        my_env = os.environ.copy()
+        self.pypath: str = my_env.get("PYTHONPATH", os.getcwd())
+        self.base_mypy_call: List[str] = [sys.executable, "-m", "mypy", "--ignore-missing-imports"]
+
+
+if __name__ == '__main__':
+    unittest.main()