Ruff CI/CD integration

[Andrei-Treil]

#17

[davidgantman]

Mark the PR as a draft until you're ready for review.

[davidgantman]

Make sure to rebase your changes, when you're ready for review.

[davidgantman]

Commit: "Move LLM prompts to markdown files" should be in a separate PR with a corresponding issue

[davidgantman]

No way to default to latest?

[Andrei-Treil]

Defaulting to latest isn't supported, instead you're meant to run pre-commit autoupdate which will update the pre-commit dependencies to their latest versions. This could be automated through a scheduled GitHub action or using pre-commit ci

[davidgantman]

I'm not sure if signing off is supposed to be automatic, my understanding was that signing off is the developer's way of acknowledging the DCO.

[Andrei-Treil]

I can remove the auto add feature and just have it block the commit instead

[davidgantman]

I don't want to block the commit, is it possible to just give a warning that they can ignore? The missing SOB will still be caught and blocked when reviewing the PR.

[Andrei-Treil]

In theory the error acts as a warning, since the pre-commit checks can be avoided by running git commit -m "Test" --no-verify.

I could make it just log the warning, but this would be missed if using non-cli for Git (for example the VSCode source control extension):

git commit -m "Test"                     
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/andrei/.cache/pre-commit/patch1754334020-138083.
ruff check...........................................(no files to check)Skipped
ruff format..........................................(no files to check)Skipped
[INFO] Restored changes from /home/andrei/.cache/pre-commit/patch1754334020-138083.
[WARNING] Unstaged files detected.
[INFO] Stashing unstaged files to /home/andrei/.cache/pre-commit/patch1754334020-138102.
ruff check...........................................(no files to check)Skipped
ruff format..........................................(no files to check)Skipped
Check commit signoff.....................................................Passed
- hook id: check-signoff
- duration: 0s

WARNING: Missing Signed-off-by line
Use: git commit -s

[INFO] Restored changes from /home/andrei/.cache/pre-commit/patch1754334020-138102.
[import-adjustment 1bde428] Test
 1 file changed, 3 insertions(+), 7 deletions(-)

[davidgantman]

Good point, and thanks for the example? What do other projects do? I think blocking the commit is fine like you said, to make sure that GUI clients still block the commit so that users don't need to deal with going back and amending their changes for something minimal like SOB.

[Andrei-Treil]

From what I can tell, most projects just run the DCO check on PRs through GitHub actions. In instances where DCO check is included in a pre-commit hook, the common behavior is to block the commit.

[davidgantman]

Let's do the common behavior for including the DCO check. I'm fine with blocking as long as we mention --force can bypass the precommit checks in CONTRIBUTING.md. Make sure that it's easy to understand how to bypass so that potential contributors are aware that they can bypass it if they choose to do so, rather than being frustrated at the project for enforcing it.

[davidgantman]

Can you also add markdown linting?

[Andrei-Treil]

What rules do we want to enforce for markdown?

[davidgantman]

Whichever rules are enforced by the markdownlint extension on VS Code is good for now.

[davidgantman]

"[Add dev dependancy install]"(c3c4c1b)
s/dependancy/dependency/ 😜

Also when you're ready for a proper review, can you squash commits wherever it makes sense to do so? e.g. "Fix actions install" should be squashed, no reason to merged a change and it's fix separately.

[Andrei-Treil]

@davidgantman

Commit: "Move LLM prompts to markdown files" should be in a separate PR with a corresponding issue

Moving the prompts to markdown files was done as part of fixing the existing formatting issues, as the prompts exceed the maximum length of Python lines (88 characters). Since white space in prompts can impact LLM behavior, I felt it best to refactor the prompts in this way instead of editing the string definitions in the Python files.

This PR seeks to add style guidelines and update the codebase to match these guidelines. Since the motivation for this change is to adhere to the new style guidelines, I think adding it here is appropriate.

LMK if you disagree, I can instead separate this into a different PR and then rebase this PR once the changes are merged.

[davidgantman]

nit: In your git config, your user should be rather than your handle,

[davidgantman]

LGTM, will merge after testing locally.

[davidgantman]

It looks like Code Quality tests are failing

[Andrei-Treil]

It looks like Code Quality tests are failing

It's the markdown linting line length check (for future reference you can view the exact failing cases in the details of the Github Actions run). I can add a quick change to ignore the rule if you'd like, otherwise I would have to rewrite README and CONTRIBUTING, which seems unnecessary.

[davidgantman]

It looks like Code Quality tests are failing

It's the markdown linting line length check (for future reference you can view the exact failing cases in the details of the Github Actions run). I can add a quick change to ignore the rule if you'd like, otherwise I would have to rewrite README and CONTRIBUTING, which seems unnecessary.

Okay let's drop markdown linting check from this PR then. Once the README.md and CONTRIBUTING.md pass the markdown lint check, we can merge it.

[Andrei-Treil]

It looks like Code Quality tests are failing

It's the markdown linting line length check (for future reference you can view the exact failing cases in the details of the Github Actions run). I can add a quick change to ignore the rule if you'd like, otherwise I would have to rewrite README and CONTRIBUTING, which seems unnecessary.

Okay let's drop markdown linting check from this PR then. Once the README.md and CONTRIBUTING.md pass the markdown lint check, we can merge it.

To clarify, do you want me to remove the markdown linting entirely from this PR? Additionally, does this mean that we do want to enforce line length rules for markdown files?

[davidgantman]

It looks like Code Quality tests are failing

It's the markdown linting line length check (for future reference you can view the exact failing cases in the details of the Github Actions run). I can add a quick change to ignore the rule if you'd like, otherwise I would have to rewrite README and CONTRIBUTING, which seems unnecessary.

Okay let's drop markdown linting check from this PR then. Once the README.md and CONTRIBUTING.md pass the markdown lint check, we can merge it.

To clarify, do you want me to remove the markdown linting entirely from this PR? Additionally, does this mean that we do want to enforce line length rules for markdown files?

Unless we can disable the line length rule for markdown files, yes, let's remove markdown linting from this PR. We can create a separate PR for it that includes fixes to README.md and CONTRIBUTING.md.

[Andrei-Treil]

It looks like Code Quality tests are failing

It's the markdown linting line length check (for future reference you can view the exact failing cases in the details of the Github Actions run). I can add a quick change to ignore the rule if you'd like, otherwise I would have to rewrite README and CONTRIBUTING, which seems unnecessary.

Okay let's drop markdown linting check from this PR then. Once the README.md and CONTRIBUTING.md pass the markdown lint check, we can merge it.

To clarify, do you want me to remove the markdown linting entirely from this PR? Additionally, does this mean that we do want to enforce line length rules for markdown files?

Unless we can disable the line length rule for markdown files, yes, let's remove markdown linting from this PR. We can create a separate PR for it that includes fixes to README.md and CONTRIBUTING.md.

Yes line length can be disabled for only markdown files, its a 1 line change to .markdownlint.json:

"MD013": false, // Disable rule MD013 (line length)

[Andrei-Treil]

Added the change, not sure why the code quality check isn't appearing in the logs -- debugging rn, will update you when done. Re: conflict resolution, can you confirm that the kernel dir (/patch_review/kernel/) should be removed from .gitignore?

[davidgantman]

Added the change, not sure why the code quality check isn't appearing in the logs -- debugging rn, will update you when done. Re: conflict resolution, can you confirm that the kernel dir (/patch_review/kernel/) should be removed from .gitignore?

Yes, patch_review/kernel is an outdated reference, it should be removed from .gitignore 👍

[github-actions]

This pull request has been marked as stale due to 60 days of inactivity. To prevent automatic closure in 10 days, remove the stale label or add a comment. You can reopen a closed pull request at any time.