From 1674ad54d4d1566b6ca18cd2fd2b643e0211dd05 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 20:22:18 +0000 Subject: [PATCH 01/11] refactor: get_issue_url() returns *just* the URL Leave the extra text to the caller (main) --- GitHubClient.py | 2 +- main.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/GitHubClient.py b/GitHubClient.py index dea1daa..f38db91 100644 --- a/GitHubClient.py +++ b/GitHubClient.py @@ -352,4 +352,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}' diff --git a/main.py b/main.py index b0dd63f..a16f753 100644 --- a/main.py +++ b/main.py @@ -101,7 +101,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: From 02c158cabe7d16d037c368e125df25047714746f Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 03:30:48 +0000 Subject: [PATCH 02/11] refactor: abstract diff method within client There's no reason to have main directly construct the URL. A different VCS (such as GitLab) could have a different URL construction. Further, in the case of LocalClient, there is no URL. --- GitHubClient.py | 20 ++++++++++++++++++-- LocalClient.py | 25 ++++++++++++++++++++++++- main.py | 21 +++++---------------- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/GitHubClient.py b/GitHubClient.py index f38db91..15db432 100644 --- a/GitHubClient.py +++ b/GitHubClient.py @@ -17,7 +17,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' @@ -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', diff --git a/LocalClient.py b/LocalClient.py index 9d0b606..b15a495 100644 --- a/LocalClient.py +++ b/LocalClient.py @@ -1,13 +1,36 @@ import subprocess +import os class LocalClient(object): 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') + return subprocess.run(['git', 'diff', f'{self.base_ref}..{self.sha}'], stdout=subprocess.PIPE).stdout.decode('latin-1') def create_issue(self, issue): return [201, None] diff --git a/main.py b/main.py index a16f753..63552d0 100644 --- a/main.py +++ b/main.py @@ -25,23 +25,12 @@ if __name__ == "__main__": # 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. From 1fa135d86b39ad1c27d9a0ba89e8c9131a4bcb31 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 20:27:12 +0000 Subject: [PATCH 03/11] feat: provide additional info about issue being processed --- main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/main.py b/main.py index 63552d0..f02fe32 100644 --- a/main.py +++ b/main.py @@ -75,7 +75,7 @@ if __name__ == "__main__": # 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: From a4edef676c576872c03e4ad7d2ba993991e4173e Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 21:32:10 +0000 Subject: [PATCH 04/11] feat: add get_issue_url() This is really just a dummy operation so that main can unconditionally make the call on any client --- LocalClient.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/LocalClient.py b/LocalClient.py index b15a495..adbb912 100644 --- a/LocalClient.py +++ b/LocalClient.py @@ -37,3 +37,6 @@ class LocalClient(object): def close_issue(self, issue): return 200 + + def get_issue_url(self, new_issue_number): + return "N/A" From 21f86f529e0e4794f66585d4e8b71a7acac03790 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 22:16:11 +0000 Subject: [PATCH 05/11] feat: provide more info about issue create/update --- main.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index f02fe32..0462e3e 100644 --- a/main.py +++ b/main.py @@ -79,7 +79,7 @@ if __name__ == "__main__": if raw_issue.status == LineStatus.ADDED: status_code, new_issue_number = client.create_issue(raw_issue) if status_code == 201: - print('Issue created') + print(f'Issue created: : #{new_issue_number} @ {client.get_issue_url(new_issue_number)}') # Check to see if we should insert the issue URL back into the linked TODO. # 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('#')): @@ -98,7 +98,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': From bd01636b00d4229741ce7240b153be9edbba7a26 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Mon, 28 Oct 2024 15:21:38 +0000 Subject: [PATCH 06/11] refactor: move insert_issue_urls logic to main --- GitHubClient.py | 1 - main.py | 6 ++++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/GitHubClient.py b/GitHubClient.py index 15db432..0c56049 100644 --- a/GitHubClient.py +++ b/GitHubClient.py @@ -34,7 +34,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: diff --git a/main.py b/main.py index 0462e3e..676a5cb 100644 --- a/main.py +++ b/main.py @@ -73,6 +73,9 @@ 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)}: '{raw_issue.title}' @ {raw_issue.file_name}:{raw_issue.start_line}") @@ -80,9 +83,8 @@ if __name__ == "__main__": status_code, new_issue_number = client.create_issue(raw_issue) if status_code == 201: print(f'Issue created: : #{new_issue_number} @ {client.get_issue_url(new_issue_number)}') - # Check to see if we should insert the issue URL back into the linked TODO. # 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() From eefdda59f9431015beaeb21b0bd8cc3a3d81bf34 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Sat, 26 Oct 2024 03:31:26 +0000 Subject: [PATCH 07/11] chore: add devcontainer --- .devcontainer/devcontainer.json | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .devcontainer/devcontainer.json diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json new file mode 100644 index 0000000..ce3acb4 --- /dev/null +++ b/.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" + + // Configure tool-specific properties. + // "customizations": {}, + + // Uncomment to connect as root instead. More info: https://aka.ms/dev-containers-non-root. + // "remoteUser": "root" +} From 7435acbd70f20e49bb2ec8203789aa1e98d269a2 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Fri, 25 Oct 2024 15:56:44 +0000 Subject: [PATCH 08/11] feat: add additional Dockerfile target Add additional, non-default, Dockerfile target which installs app into an ubuntu image. This is useful for local development and enables other operations (such as committing back to the repo) to occur within the container. --- Dockerfile | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/Dockerfile b/Dockerfile index 5ab27cf..184cec7 100644 --- a/Dockerfile +++ b/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 From ef72f61bedab1f9241768f686ac8d8afc141e95d Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Mon, 28 Oct 2024 17:59:15 +0000 Subject: [PATCH 09/11] refactor: address mypy type warning --- main.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/main.py b/main.py index 676a5cb..f73f9b1 100644 --- a/main.py +++ b/main.py @@ -36,9 +36,9 @@ if __name__ == "__main__": # 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) From f1f17d83720da0ce5e7b03f4ab0c016ebad1705e Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Mon, 28 Oct 2024 18:01:56 +0000 Subject: [PATCH 10/11] refactor: add common base class to GitHubClient and LocalClient Enables adding type checking to minimize mypy errors --- Client.py | 12 ++++++++++++ GitHubClient.py | 3 ++- LocalClient.py | 12 ++---------- main.py | 4 +++- 4 files changed, 19 insertions(+), 12 deletions(-) create mode 100644 Client.py diff --git a/Client.py b/Client.py new file mode 100644 index 0000000..5d9fdf6 --- /dev/null +++ b/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" diff --git a/GitHubClient.py b/GitHubClient.py index 0c56049..4e3dc28 100644 --- a/GitHubClient.py +++ b/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 = [] diff --git a/LocalClient.py b/LocalClient.py index adbb912..4c8df17 100644 --- a/LocalClient.py +++ b/LocalClient.py @@ -1,7 +1,8 @@ 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 @@ -31,12 +32,3 @@ class LocalClient(object): def get_last_diff(self): return subprocess.run(['git', 'diff', f'{self.base_ref}..{self.sha}'], stdout=subprocess.PIPE).stdout.decode('latin-1') - - 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" diff --git a/main.py b/main.py index f73f9b1..049dc57 100644 --- a/main.py +++ b/main.py @@ -10,18 +10,20 @@ 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() From 649e50253b421d08e72ba29100d7afc0efeb0559 Mon Sep 17 00:00:00 2001 From: Robert Alonso <17463757+rgalonso@users.noreply.github.com> Date: Mon, 28 Oct 2024 18:28:52 +0000 Subject: [PATCH 11/11] test: add unittest to run mypy New unittest file consists of two tests. Both run mypy, but one does so with arguments that are expected to allow the test to pass. The other does not provide those arguments and is expected to fail. The expected failure is intentional, to serve as an ongoing reminder to try to 1) move towards having strict typing in the codebase and to 2) not add any additional errors/warnings in the meantime. Separated requirements into runtime (requirements.txt) and additional development (requirements-dev.txt). Modified devcontainer and workflow to reflect the change. --- .devcontainer/devcontainer.json | 2 +- .github/workflows/ci.yml | 2 +- requirements-dev.txt | 4 ++++ requirements.txt | 3 +-- tests/test_syntax.py | 39 +++++++++++++++++++++++++++++++++ 5 files changed, 46 insertions(+), 4 deletions(-) create mode 100644 requirements-dev.txt create mode 100644 tests/test_syntax.py diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index ce3acb4..9ab1cda 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -12,7 +12,7 @@ // "forwardPorts": [], // Use 'postCreateCommand' to run commands after the container is created. - "postCreateCommand": "pip3 install --user -r requirements.txt" + "postCreateCommand": "pip3 install --user -r requirements.txt -r requirements-dev.txt" // Configure tool-specific properties. // "customizations": {}, diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53d1400..412ba54 100644 --- a/.github/workflows/ci.yml +++ b/.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 \ No newline at end of file diff --git a/requirements-dev.txt b/requirements-dev.txt new file mode 100644 index 0000000..8db956a --- /dev/null +++ b/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 \ No newline at end of file diff --git a/requirements.txt b/requirements.txt index 30ce159..af01943 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,3 +1,2 @@ requests==2.32.3 -ruamel.yaml==0.18.6 -pytest==8.3.3 \ No newline at end of file +ruamel.yaml==0.18.6 \ No newline at end of file diff --git a/tests/test_syntax.py b/tests/test_syntax.py new file mode 100644 index 0000000..76ae99f --- /dev/null +++ b/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()