Skip to content

feat: add automatic linting suggestion github workflow#46

Merged
bretbrownjr merged 1 commit intobemanproject:mainfrom
linusboehm:git_workflow_lint_suggestions
Oct 17, 2024
Merged

feat: add automatic linting suggestion github workflow#46
bretbrownjr merged 1 commit intobemanproject:mainfrom
linusboehm:git_workflow_lint_suggestions

Conversation

@linusboehm
Copy link
Copy Markdown
Contributor

this commit adds a github workflow to automatically run pre-commit and suggests changes to a PR, to fix any linting errors. An example error + suggested fix can be seen here: linusboehm#2
Suggestions can be accepted + committed from the PR on github.

@wusatosi
Copy link
Copy Markdown
Member

Oh wow, this is fantastic! I have never heard reviewdog before but this is fantastic!

Do this report problems for files that doesn't get auto-formatted? (e.g. markdown files)

@camio
Copy link
Copy Markdown
Member

camio commented Oct 15, 2024

For what this does, there is surprisingly little configuration we'd have to maintain. The reviewdog project looks actively maintained as well. I'm very much in favor of moving forward with this.

#34 is merged now. I think this PR needs to be rebased on top of that to get this PR merged as well.

@linusboehm are there any drawbacks to moving ahead with this PR that we're not seeing? It looks like only upside to me.

Copy link
Copy Markdown
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for bringing-in reviewdog, this functionality looks fantastic.

Since #34 is merged, you can simply add the review dog configuration onto .github/workflows/pre-commit.yml.

Comment thread .github/workflows/reviewdog.yml
Comment on lines +5 to +9
permissions:
contents: read
checks: write
issues: write
pull-requests: write
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to move this as well, I don't think the default actions permission allows writes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean move it to .github/workflows/pre-commit.yml? Are the permissions set for that workflow already?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohno! Sorry I didn't reply to this comment before merging, all the permissions are needed in pre-commit.yml.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-introduced these permissions at #50

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again sorry for not being clear.

Comment thread .github/workflows/reviewdog.yml
@linusboehm
Copy link
Copy Markdown
Contributor Author

@linusboehm are there any drawbacks to moving ahead with this PR that we're not seeing? It looks like only upside to me.

The only time I've tried to use this was in a codebase with the requirement that only changed lines are formatted (instead of whole files) as the codebase didn't have consistent formatting. That seemed to be a bit cumbersome and sometimes lead to formatting changes that obfuscated the underlying code change. Given that beman repos should be formatted consistently according to the format spec, this shouldn't be a problem and I can't think of any other reasons not to give it a try.

I'll rebase and address the review comments soon (@wusatosi thanks for the review).

@linusboehm linusboehm force-pushed the git_workflow_lint_suggestions branch 3 times, most recently from d00c82b to 581b5e5 Compare October 17, 2024 01:03
@linusboehm linusboehm force-pushed the git_workflow_lint_suggestions branch from 581b5e5 to a7e1a0f Compare October 17, 2024 01:23
@linusboehm linusboehm requested a review from wusatosi October 17, 2024 01:26
@bretbrownjr bretbrownjr merged commit 1ba4f15 into bemanproject:main Oct 17, 2024
Copy link
Copy Markdown
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pr!

with:
python-version: 3.13

- name: Cache pre-commit hooks
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes with pre-commit/action and is actually not needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it set the paths to cache when not explicitly specifying them?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-commit/action caches based on the default configuration location.

Usually these actions come with appropriate caching built-in (not the pip install pre-commit, but oh well).

e.g. see action log:
https://github.com/beman-project/exemplar/actions/runs/11385583999/job/31675772106#step:6:1

See also:
https://github.com/pre-commit/action/blob/main/action.yml

I guess leaving this in is fine, the restored cache by this step will be overwritten/ merged in with that from pre-commit/action step so really no harm is done. ¯⁠\⁠_⁠(⁠ツ⁠)⁠_⁠/⁠¯

- uses: pre-commit/[email protected]
with:
extra_args: --files ${{ steps.changed-files.outputs.all_changed_files }}
continue-on-error: true

This comment was marked as resolved.

Copy link
Copy Markdown
Member

@wusatosi wusatosi Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in-fact needed. I am wrong.

@wusatosi
Copy link
Copy Markdown
Member

Ops didn't realize this is already merged

This was referenced Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants