refactor in preparation for supporting other VCSs #217

rgalonso · 2024-10-25T00:21:19Z

It's important to start off by saying that this PR shouldn't produce any behavioral changes. It just splits the various classes defined in main.py into their own modules. So why make the change?

This is in preparation for future PRs to support other version control systems (VCS). In particular, I'm interested in supporting GitLab, but I don't see why this couldn't be extended to others, such as BitBucket/Jira. In fact, I've already adapted this project to support GitLab and have started using it on a self-hosted instance. I don't yet have feature parity so it's not ready to be merged though. But I wanted to go ahead and try to get this refactor worked in ASAP to ensure that our codebases don't drift too far.

Besides splitting the classes off, there are just a couple of minor changes being made here. Namely, I've added a LocalClient class. I found this to be helpful for local testing and it demonstrates how we can have a generic *Client interface that is implemented as needed for GitHub, GitLab, etc. I've also tweaked the GitHub client constructor to make it easier to fall back to other clients in the future.

In the few places where the code is not a straight copy of what was originally in main.py, I'll note it with inline comments.

Thanks for this great project. I look forward to hopefully being able to make it even better!

Split main.py into modules to (mostly) isolate the GitHub-specific code from the general TODO detection and program logic. This is both for readability/maintainability and to prepare for potentially supporting other version control systems (e.g. GitLab, BitBucket).

This is just a safe fallback for local testing. If environment variables which activate the creation of the GitHub client aren't set, then the Local client is created. It acts on the most recent commit of the repo in the working directory. Minor edit to GitHubClient so that it raises an EnvironmentError exception if INPUT_GITHUB_URL environment variable is not defined. This allows main to detect the error and fall back to trying to use the Local client

Add a new method, get_issue_url(), which returns the VCS-specific web URL for viewing an issue. This results in moving GitHub-specific code from main into GitHubClient, but with no change in behavior.

rgalonso · 2024-10-25T00:23:27Z

GitHubClient.py

+        if not self.github_url:
+            raise EnvironmentError


Error out early if GitHub-specific environment variable is not set. The exception is caught in main, where it used to fall back to other options.

rgalonso · 2024-10-25T00:32:25Z

GitHubClient.py

+        self.repo = os.getenv('INPUT_REPO')
+        self.before = os.getenv('INPUT_BEFORE')
+        self.sha = os.getenv('INPUT_SHA')
+        self.commits = json.loads(os.getenv('INPUT_COMMITS')) or []


Just FYI, but I found that this line raises the following TypeError when INPUT_COMMITS is undefined.

TypeError: the JSON object must be str, bytes or bytearray, not NoneType

I didn't want to rely on that error path though, which is why I added the segment above at lines 12-13.

Since it's not relevant to this PR, I didn't include the fix. But if you want to clean this up, this does what you were maybe intending to do:

self.commits = json.loads(os.getenv('INPUT_COMMITS', str([])))

Never noticed that before, thanks! Yes, we should incorporate this.

rgalonso · 2024-10-25T00:34:26Z

GitHubClient.py

+    def get_issue_url(self, new_issue_number):
+        return f'Issue URL: {self.line_base_url}{self.repo}/issues/{new_issue_number}'


The syntax of this URL is different between GitHub and GitLab, so I pulled into the class (as a new method) rather than leaving it in main.

rgalonso · 2024-10-25T00:36:47Z

main.py

+    # 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
+    # if needed, fall back to using a local client for testing
+    client = client or LocalClient()
+


Rather than unconditionally trying to create a GitHub client (and failing with an ugly uncaught exception when you can't), this now catches the situation where the GitHub client can't be constructed and falls back to using a (very simply) Local client.

rgalonso · 2024-10-25T00:37:51Z

main.py

@@ -990,7 +102,7 @@ def _should_ignore(self, file):
                            # Duplicate the line to retain the comment syntax.
                            new_line = file_lines[line_number]
                            remove = fr'{raw_issue.identifier}.*{raw_issue.title}'
-                            insert = f'Issue URL: {client.line_base_url}{client.repo}/issues/{new_issue_number}'
+                            insert = client.get_issue_url(new_issue_number)


As noted in my earlier comment, this is just to pull the GitHub-specific URL format out of this function. Each client is now responsible for implementing this method as appropriate for their system.

alstr · 2024-10-25T12:01:32Z

Great idea, thanks very much for the work!

I'll have a quick scan over things but it all seems good to me. We'll probably need to update the tests as well, specifically: from main import TodoParser

rgalonso · 2024-10-25T13:03:07Z

Great idea, thanks very much for the work!

I'll have a quick scan over things but it all seems good to me. We'll probably need to update the tests as well, specifically: from main import TodoParser

I was surprised, but that still works as is. I think it's because main already pulls everything else in. That said, it's probably better to be explicit about things and do the imports in the test file.

On a related note, I found that running python -m unittest (as shown in the README) produces a bunch of errors. That was even the case before I touched it, so it doesn't have to do with my changes. But curiously, if I instead run python -m unittest -k test_name for each test, then it all passes. Any idea what's going on? I suspect something is different between my local environment and yours, but I haven't been able to figure it out.

If you'd like me to share logs, just say the word.

alstr · 2024-10-25T13:27:44Z

Great idea, thanks very much for the work!
I'll have a quick scan over things but it all seems good to me. We'll probably need to update the tests as well, specifically: from main import TodoParser

I was surprised, but that still works as is. I think it's because main already pulls everything else in. That said, it's probably better to be explicit about things and do the imports in the test file.

On a related note, I found that running python -m unittest (as shown in the README) produces a bunch of errors. That was even the case before I touched it, so it doesn't have to do with my changes. But curiously, if I instead run python -m unittest -k test_name for each test, then it all passes. Any idea what's going on? I suspect something is different between my local environment and yours, but I haven't been able to figure it out.

If you'd like me to share logs, just say the word.

I think there are one or more issues depending on what the current working directory is set to, specifically with the CustomLanguageTest. I think it's looking for /tests/tests/custom_languages.json due to this line in main.py:

path = os.path.join(os.getcwd(), path)

rgalonso · 2024-10-25T13:40:27Z

I think there are one or more issues depending on what the current working directory is set to, specifically with the CustomLanguageTest. I think it's looking for /tests/tests/custom_languages.json due to this line in main.py:

path = os.path.join(os.getcwd(), path)

Yeah, I thought that could be the case, but I'm not sure it is. My current working directory is the clone root (the one containing main.py, etc.) I searched for the line you called out (it's now found at TodoParser.py:84). I set a breakpoint there and ran unittest again. ~~The breakpoint doesn't even get hit.~~ (Update: Ran it again and it does get hit. Not sure what happened the first time...)

Anyway, that's kind of a tangential conversation that is probably outside of the scope of this PR. If you do want to continue this conversation though (and I'm happy to do so), maybe it belongs over in Discussions? Since the tests are still passing for me when I run them one-by-one, I don't want to derail this PR over this unittest issue.

rgalonso · 2024-10-25T14:16:27Z

I figured out the problem with the unittest. You were on the right track about the CustomLanguageTest. It didn't have to do with the working directory though. It was that the test was modifying the INPUT_LANGUAGES and INPUT_NO_STANDARD environment variables. When running tests one-by-one, that's fine. But when running all of the tests together, that corrupted the environment for all tests that happen to follow it.

I've already got the fix in place. Do you want me to include it in this PR, or should I want until this PR is merged since the unittest issue is its own thing. Let me know. Thanks.

alstr · 2024-10-25T14:53:59Z

Oh great. Yeah, let's sort it separately to keep things on track.

So back to the original PR, everything looks good, I'm just going to test a few things and make sure it's all running smoothly, but seems fine from here.

alstr · 2024-10-25T17:37:15Z

All seems great, just a few imports that can be cleaned up but that's it. Do you want to do that then I'll merge? Thanks! 😄

rgalonso · 2024-10-25T17:40:23Z

All seems great, just a few imports that can be cleaned up but that's it. Do you want to do that then I'll merge? Thanks! 😄

I'm happy to do it, but I'm not sure what you're asking for. If you've already done it locally, then feel free to push the changes you're talking about. Otherwise, just let me know exactly what needs done and I'll make the change.

alstr · 2024-10-25T17:53:50Z

All seems great, just a few imports that can be cleaned up but that's it. Do you want to do that then I'll merge? Thanks! 😄

I'm happy to do it, but I'm not sure what you're asking for. If you've already done it locally, then feel free to push the changes you're talking about. Otherwise, just let me know exactly what needs done and I'll make the change.

I think there were just a few unused imports in some of the files; (from memory) requests isn't used in TodoParser.py. If you don't do it I'll have a look next week. 😄

rgalonso · 2024-10-25T18:13:21Z

All seems great, just a few imports that can be cleaned up but that's it. Do you want to do that then I'll merge? Thanks! 😄

I'm happy to do it, but I'm not sure what you're asking for. If you've already done it locally, then feel free to push the changes you're talking about. Otherwise, just let me know exactly what needs done and I'll make the change.

I think there were just a few unused imports in some of the files; (from memory) requests isn't used in TodoParser.py. If you don't do it I'll have a look next week. 😄

Done!

vscode ➜ /workspaces/todo-to-issue-action (feat/prep-generic-vcs-support) $ git rev-parse HEAD
9b24f44c630cdf4403e20bb37754220b533fd99d
vscode ➜ /workspaces/todo-to-issue-action (feat/prep-generic-vcs-support) $ flake8 --select=F401 $(git ls-files \*.py)
vscode ➜ /workspaces/todo-to-issue-action (feat/prep-generic-vcs-support) $

alstr · 2024-10-25T18:44:42Z

Thanks! Just removed a few more that I noticed.

rgalonso · 2024-10-25T18:56:47Z

Thanks! Just removed a few more that I noticed.

Yeah, I see that. It's interesting that flake8 didn't seem to catch it. Oh well. Thanks!

BTW, I've just submitted the other PR we discussed to address the unittest issue.

rgalonso · 2024-10-25T19:11:59Z

Sorry to say it, but the change in main.py broke the code. It really does rely on os and re. I'm resubmitting the PR with that change reverted.

alstr · 2024-10-28T09:25:23Z

Huh. The tests ran fine but must not have picked up this issue. Thanks for sorting it.

rgalonso · 2024-10-28T15:00:59Z

Huh. The tests ran fine but must not have picked up this issue. Thanks for sorting it.

I think the problem is that the tests are currently just exercising TodoParser. There's a fair amount of logic in main, GitHubClient (and now LocalClient and hopefully soon GitLabClient) that's going untested.

I recognize it could be a bit of a big lift to put in tests for all of that, but maybe a quick middle ground is to use mypy instead. Locally, I just intentionally introduced an error and it caught it. Since mypy's aim is to layer static typing onto Python, it issued other warnings and errors as well, but with the right arguments I was able to restrict the output so that it can still be treated as dynamically typed. If you'd like to discuss this further (here or in Discussions), let me know.

alstr · 2024-10-28T16:53:01Z

Sounds good; improving the tests is something I've been hoping to do. If you have any ideas or time to spare to look into it by all means.

rgalonso added 3 commits October 25, 2024 00:07

refactor: move GitHub-specific code into class

c05a9e3

Add a new method, get_issue_url(), which returns the VCS-specific web URL for viewing an issue. This results in moving GitHub-specific code from main into GitHubClient, but with no change in behavior.

rgalonso commented Oct 25, 2024

View reviewed changes

rgalonso marked this pull request as ready for review October 25, 2024 00:39

fix: copy all modules into Docker image

3753bae

refactor: clean up imports

9b24f44

alstr added 2 commits October 25, 2024 19:42

Update TodoParser.py

d6553db

Update main.py

2157658

alstr merged commit 22ab650 into alstr:master Oct 25, 2024
1 check passed

rgalonso mentioned this pull request Oct 25, 2024

Revert "Update main.py" #221

Merged

rgalonso mentioned this pull request Oct 28, 2024

refactor and add tests #222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor in preparation for supporting other VCSs #217

refactor in preparation for supporting other VCSs #217

rgalonso commented Oct 25, 2024

rgalonso Oct 25, 2024

rgalonso Oct 25, 2024

alstr Oct 25, 2024 •

edited

Loading

rgalonso Oct 25, 2024

rgalonso Oct 25, 2024

rgalonso Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024 •

edited

Loading

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 28, 2024

rgalonso commented Oct 28, 2024

alstr commented Oct 28, 2024

		def get_issue_url(self, new_issue_number):
		return f'Issue URL: {self.line_base_url}{self.repo}/issues/{new_issue_number}'

refactor in preparation for supporting other VCSs #217

refactor in preparation for supporting other VCSs #217

Conversation

rgalonso commented Oct 25, 2024

rgalonso Oct 25, 2024

Choose a reason for hiding this comment

rgalonso Oct 25, 2024

Choose a reason for hiding this comment

alstr Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

rgalonso Oct 25, 2024

Choose a reason for hiding this comment

rgalonso Oct 25, 2024

Choose a reason for hiding this comment

rgalonso Oct 25, 2024

Choose a reason for hiding this comment

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024 • edited Loading

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 25, 2024

rgalonso commented Oct 25, 2024

rgalonso commented Oct 25, 2024

alstr commented Oct 28, 2024

rgalonso commented Oct 28, 2024

alstr commented Oct 28, 2024

alstr Oct 25, 2024 •

edited

Loading

rgalonso commented Oct 25, 2024 •

edited

Loading