refactor and add tests

[rgalonso]

This contains some additional minor refactoring and adds another unittest file.

refactor-related changes

The following changes were made further isolate the VCS-specific code from that in main.

• added abstract Client base class
  • this also supports mypy-related changes, discussed below
• parse MANUAL_COMMIT_REF and MANUAL_BASE_REF environment variables within *Client so that diff URL can be VCS-specific
  • for LocalClient, there is no URL, but parsing these variables allows it to call git diff with the proper arguments
• Client:get_issue_url() returns just the URL string (when applicable), rather than prefixing it with "Issue URL: ". This allows the function to be used more generically.

more thorough testing

In addition, per this discussion in PR #217, I added an additional test file which uses mypy to look for additional errors that weren't being caught before. mypy is really intended for layering static typing on top of Python. That's a nice goal, but I've limited the changes here to most just focus on having the additional testing without being concerned about the typing.

other minor items

• added devcontainer configuration
  • to support local development
• added additional (non-defualt) Dockerfile target
  • to support local testing and upcoming GitLab integration
• minor changes to requirements/-dev.txt and GitHub workflow to support above changes

[alstr]

This is awesome. Thanks very much for the effort. I'll take a look over it shortly!

[rgalonso]

This is awesome. Thanks very much for the effort. I'll take a look over it shortly!

In particular, please pay attention to how this is working for GitHub Actions. Since I'm focused on GitLab, I'm not testing the GitHub side of it all. I'm not even sure how to! So I'm definitely relying on you and any existing regression tests to make sure that side of it is still working properly. (You can probably take that as a general note as I continue to submit new PRs.) Thanks again!

[alstr]

All looks good, thanks again. About the update to the Dockerfile; what's the purpose of the additional stage? Just wondering if it'll slow things down.

[rgalonso]

That's a fair concern, but with modern Docker it's not an issue. It used to be that it would build all stages, but these days it's smart enough to skip the new stage I introduced because it can see that the final (unnamed) stage doesn't rely on it. As to why I introduced it, it's needed for GitLab CI/CD. Independent of that, I've also found it useful for local testing prior to a push. Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Alastair Mooney ***@***.***> Sent: Monday, November 4, 2024 7:05:36 AM To: alstr/todo-to-issue-action ***@***.***> Cc: Robert Alonso ***@***.***>; Author ***@***.***> Subject: Re: [alstr/todo-to-issue-action] refactor and add tests (PR #222) Caution: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. All looks good, thanks again. About the update to the Dockerfile; what's the purpose of the additional stage? Just wondering if it'll slow things down. — Reply to this email directly, view it on GitHub<#222 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEFHTTIK4HW7MNGRGADUH5LZ65PIZAVCNFSM6AAAAABQYDFAXGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJUGU2DAOBTGI>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[alstr]

Thanks for that.

I've merged the PR, adding a couple of other bits in the meantime:

• Added server URL to GitHubClient (Fix link generation for GitHub Enterprise by using GITHUB_SERVER_URL #218)
• Importing TodoParser from main.py was failing, so I updated it to import directly
• Client, GitHubClient, LocalClient, LineStatus, TodoParser were undefined in main.py so I have also imported them manually. At first I thought this was to be expected looking at test_todo_parser.py, but having looked into it further I don't think these errors were expected.

[rgalonso]

Thanks for that.

I've merged the PR, adding a couple of other bits in the meantime:

• Added server URL to GitHubClient (Fix link generation for GitHub Enterprise by using GITHUB_SERVER_URL #218)
• Importing TodoParser from main.py was failing, so I updated it to import directly
• Client, GitHubClient, LocalClient, LineStatus, TodoParser were undefined in main.py so I have also imported them manually. At first I thought this was to be expected looking at test_todo_parser.py, but having looked into it further I don't think these errors were expected.

I think the reason the TodoParser import was failing is that you may have had a bad merge conflict when you merged master to ghe-support prior to the PR #218 merge. Was that a conflict you had to fix manually? Or did the automerge mess up? It looks like the changes from master to main.py (in particular those of commit 662435d) mostly made it into the merge commit, but not entirely. Most of the main.py changes of 662435d were deletions, but there were a few critical additions (below), and those are not found in the merge commit.

+from TodoParser import *
+from LineStatus import *
+from GitHubClient import *

Yes, the check failed, but the reason was because main.py didn't have the above lines, so from main import TodoParser couldn't possibly work. Had the main.py merge gone correctly, there wouldn't have been an issue.

Thankfully, it looks like you caught it and fixed it in efb8b98, so I'm probably not telling you anything you don't know. But in case you didn't realize this is what happened, I wanted to highlight it just in case there's something about your local dev environment or process that caused this error or made you overlook the merge conflict (if there was one even reported).

--

EDIT: It's also worth noting that the new test I added in commit 649e5025 would've caught this too. See the log below from running it against commit 4b44510. So hopefully it's just a matter of making sure python -m unittest. Hope this helps.

vscode ➜ /workspaces/todo-to-issue-action (4b44510) $ python -m unittest
main.py:14: error: Name "Client" is not defined  [name-defined]
main.py:18: error: Name "GitHubClient" is not defined  [name-defined]
main.py:23: error: Name "LocalClient" is not defined  [name-defined]
main.py:30: error: Name "TodoParser" is not defined  [name-defined]
main.py:40: error: Name "LineStatus" is not defined  [name-defined]
main.py:41: error: Name "LineStatus" is not defined  [name-defined]
main.py:42: error: Name "LineStatus" is not defined  [name-defined]
main.py:43: error: Name "LineStatus" is not defined  [name-defined]
main.py:57: error: Name "LineStatus" is not defined  [name-defined]
main.py:59: error: Name "LineStatus" is not defined  [name-defined]
main.py:71: error: Name "LineStatus" is not defined  [name-defined]
main.py:79: error: Name "LineStatus" is not defined  [name-defined]
main.py:103: error: Name "LineStatus" is not defined  [name-defined]
Found 13 errors in 1 file (checked 1 source file)
Fmain.py:14: error: Name "Client" is not defined  [name-defined]
main.py:18: error: Name "GitHubClient" is not defined  [name-defined]
main.py:23: error: Name "LocalClient" is not defined  [name-defined]
main.py:30: error: Name "TodoParser" is not defined  [name-defined]
main.py:40: error: Name "LineStatus" is not defined  [name-defined]
main.py:41: error: Name "LineStatus" is not defined  [name-defined]
main.py:42: error: Name "LineStatus" is not defined  [name-defined]
main.py:43: error: Name "LineStatus" is not defined  [name-defined]
main.py:52: error: Need type annotation for "todos_status"  [var-annotated]
main.py:57: error: Name "LineStatus" is not defined  [name-defined]
main.py:59: error: Name "LineStatus" is not defined  [name-defined]
main.py:71: error: Name "LineStatus" is not defined  [name-defined]
main.py:79: error: Name "LineStatus" is not defined  [name-defined]
main.py:103: error: Name "LineStatus" is not defined  [name-defined]
Found 14 errors in 1 file (checked 1 source file)
x............................................................
======================================================================
FAIL: test_run_mypy_app (tests.test_syntax.MyPyTest.test_run_mypy_app)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/workspaces/todo-to-issue-action/tests/test_syntax.py", line 20, in test_run_mypy_app
    self.__call_mypy__(mypy_args, ["main.py"])
  File "/workspaces/todo-to-issue-action/tests/test_syntax.py", line 14, in __call_mypy__
    self.assertEqual(result, 0, '')
AssertionError: 1 != 0 : 

----------------------------------------------------------------------
Ran 62 tests in 17.869s

FAILED (failures=1, expected failures=1)

[alstr]

Yes, you're right, as the tests ran fine when I merged this PR. I'm not sure what happened. I merged this, then #218, which was pre-refactor. I did resolve the conflicts manually and thought I fixed everything highlighted, but looks like the imports got cut out somewhere along the line.

I did see the error trace from the test when I checked the action logs so that did point me in the right direction when fixing it!

Anyway all seems okay now so I'll release a new version shortly. Thanks again.