Precommit addition to the repo #82

ryandanehy · 2023-11-27T21:45:10Z

clang format style
cmake style formatting [docs]

Fixes #5
Fixes #54

pelesh · 2023-11-29T04:58:22Z

@ryandanehy, can we stage this PR? First stage would be to set up configurations. Once that is merged, we can run formatting on the code and have a separate PR to merge formatted code. Is that doable?

ryandanehy · 2023-11-29T16:37:24Z

@ryandanehy, can we stage this PR? First stage would be to set up configurations. Once that is merged, we can run formatting on the code and have a separate PR to merge formatted code. Is that doable?

Once we merge this any push to a PR that rebased on Develop will automatically format the code. I can change the CI to only run when manually triggered if that is preferred?

Essentially options are manual trigger or automatically with every push to a PR.

ryandanehy · 2023-11-29T23:54:06Z

@pelesh I think this is good to go with the merge. Then I will open a new pr with all the precommit suggested code changes. The precommit ci fails currently because it thinks the code is not format correctly so it is doing it jobs correctly.

pelesh · 2023-11-30T01:12:38Z

@pelesh I think this is good to go with the merge. Then I will open a new pr with all the precommit suggested code changes. The precommit ci fails currently because it thinks the code is not format correctly so it is doing it jobs correctly.

Let's review. I propose we merge it after #83 and #86 are merged.

cameronrutherford

A few changes, but as well we should verify this auto-fixes changes before merging. Your pipelines are failing, and they should be abl to fix themselves.

cameronrutherford · 2023-11-30T01:17:50Z

.github/workflows/pre_commit.yaml

+    # - uses: EndBug/add-and-commit@v9.1.3
+    #   # Only need to try and commit if the action failed
+    #   if: failure()
+    #   with:
+    #     fetch: false
+    #     committer_name: GitHub Actions
+    #     committer_email: actions@github.com
+    #     message: Apply pre-commmit fixes 


I think we want pre-commit to automatically apply fixes, so lets comment this back in

cameronrutherford · 2023-11-30T01:18:20Z

.github/workflows/pre_commit.yaml

+ workflow_dispatch: 
+    inputs:
+      branch:
+        description: 'The branch to build'
+        required: true


Suggested change

workflow_dispatch:

inputs:

branch:

description: 'The branch to build'

required: true

If we just run on pull_request, doesn't that generate the desired behavior?

I was trying to set it to manual so that people could start the workflow as needed. This is in reference to #82 (comment).

Well I suggest maybe deferring the implementation of that new feature into a different PR since it doesn't seem to work here, and is unclear at the moment.

Maybe instead you could also code a GitHub bot that can run the fixes for us automatically if you really want to over-engineer this...

I still don't get this. Why not just on pull_request... Your code seems like it's non-functional as I have never seen manual pipelines before

cameronrutherford · 2023-11-30T01:19:34Z

.pre-commit-config.yaml

+ci:
+    autofix_commit_msg: |
+        [pre-commit.ci] auto fixes from pre-commit.com hooks
+
+        for more information, see https://pre-commit.ci
+    autofix_prs: true
+    autoupdate_branch: ''
+    autoupdate_commit_msg: '[pre-commit.ci] pre-commit autoupdate'
+    autoupdate_schedule: weekly
+    skip: []
+    submodules: false


I'm not familiar with this configuration section. Shouldn't we just use what's in the GitHub action to autofix, and then customize the commit message there?

I still don't think this configuration section is necessary at all. The commit message is controlled by the GitHub action, and this does nothing.

ryandanehy · 2023-11-30T17:36:21Z

@cameronrutherford I commented out the auto fixes per slaven request for this PR. See #82 (comment)

The pipelines fail because it wants to autofix but I am not letting it.

cameronrutherford · 2023-11-30T18:35:20Z

@cameronrutherford I commented out the auto fixes per slaven request for this PR. See #82 (comment)

The pipelines fail because it wants to autofix but I am not letting it.

I guess we merge this with failing pipelines then :(

pelesh · 2023-11-30T19:20:01Z

@cameronrutherford I commented out the auto fixes per slaven request for this PR. See #82 (comment)
The pipelines fail because it wants to autofix but I am not letting it.

I guess we merge this with failing pipelines then :(

The other option is to enable auto checks and we merge this PR last thing before the release. Once we turn on style auto-correction all rebasing and merging will turn into a nightmare.

cameronrutherford · 2023-11-30T20:15:04Z

@cameronrutherford I commented out the auto fixes per slaven request for this PR. See #82 (comment)

The pipelines fail because it wants to autofix but I am not letting it.

I guess we merge this with failing pipelines then :(

The other option is to enable auto checks and we merge this PR last thing before the release. Once we turn on style auto-correction all rebasing and merging will turn into a nightmare.

Since you can now rebase automatically with buttons, it's not so bad. I do agree though that with PRs in motion it would be easier to merge this after those.

pelesh

Is it possible to run pre-commit locally or it runs only on GitHub?

We need at least minimal documentation in the top-level README file on how to use pre-commit before this can get merged.

Also, some tweaks to the configuration options are needed. For example, I noticed pre-commit tries to sort include directives alphabetically what is not something we really want.

ryandanehy · 2023-12-08T19:48:34Z

@pelesh I rebased, added some instructions in the main readme.md, and fixed pre-commit so that it does not sort includes. Let me your thoughts so we can get this merged.

cameronrutherford

Getting there

cameronrutherford · 2023-12-12T19:44:25Z

.github/workflows/pre_commit.yaml

+ workflow_dispatch: 
+    inputs:
+      branch:
+        description: 'The branch to build'
+        required: true


I still don't get this. Why not just on pull_request... Your code seems like it's non-functional as I have never seen manual pipelines before

cameronrutherford · 2023-12-12T19:45:05Z

.pre-commit-config.yaml

+ci:
+    autofix_commit_msg: |
+        [pre-commit.ci] auto fixes from pre-commit.com hooks
+
+        for more information, see https://pre-commit.ci
+    autofix_prs: true
+    autoupdate_branch: ''
+    autoupdate_commit_msg: '[pre-commit.ci] pre-commit autoupdate'
+    autoupdate_schedule: weekly
+    skip: []
+    submodules: false


I still don't think this configuration section is necessary at all. The commit message is controlled by the GitHub action, and this does nothing.

cameronrutherford · 2023-12-12T19:45:28Z

README.md

+### Running Pre-commit locally
+To run pre-commit locally you first must install pre-commit. It can be installed via
+```shell
+pip install pre-commit
+```
+


How do I install pip? What about Python? What if I am on a linux vs mac?

cameronrutherford · 2023-12-12T19:45:46Z

README.md

+
+To install the hooks and have pre-commit run automatically before every commit run
+```shell
+pre-commit install


Suggested change

pre-commit install

pre-commit install --install-hooks

cameronrutherford · 2023-12-12T19:46:40Z

README.md

+```shell
+pre-commit run --all-files
+```
+To learn more about pre-commit usuage check out https://pre-commit.com/#usage.


Maybe add some more information here about:

Rebasing against auto-fixes in PRs

Understanding git-flow in this context

What do I do when pre-commit fails?

Why does all this new stuff appear when I run git commit?

pelesh · 2023-12-28T18:33:37Z

I believe the failed test on Ascent is due to issue fixed in #109. Perhaps a good idea would be rebasing to develop to make sure this test passes?

more docs on pre-commit

ryandanehy · 2023-12-29T03:54:58Z

I believe the failed test on Ascent is due to issue fixed in #109. Perhaps a good idea would be rebasing to develop to make sure this test passes?

Just rebased.

cameronrutherford · 2024-01-16T18:03:19Z

pre-commit tests are still failing and no auto-fix is being applied

ryandanehy force-pushed the precommit branch 3 times, most recently from 1282316 to 741140d Compare November 28, 2023 23:04

ryandanehy force-pushed the precommit branch 2 times, most recently from daf4126 to fd3d082 Compare November 29, 2023 21:36

pelesh requested review from kswirydo, pelesh and cameronrutherford November 30, 2023 01:10

pelesh assigned ryandanehy Nov 30, 2023

pelesh marked this pull request as ready for review November 30, 2023 01:10

cameronrutherford requested changes Nov 30, 2023

View reviewed changes

pelesh requested changes Dec 3, 2023

View reviewed changes

ryandanehy force-pushed the precommit branch from da51f85 to 66c8eb8 Compare December 8, 2023 19:43

ryandanehy requested a review from pelesh December 8, 2023 21:18

cameronrutherford requested changes Dec 12, 2023

View reviewed changes

revert files

2d45845

more docs on pre-commit

ryandanehy force-pushed the precommit branch from 3fe3220 to 2d45845 Compare December 28, 2023 20:04

superwhiskers mentioned this pull request Jun 3, 2024

clarify style guidelines in CONTRIBUTING.md / set up clang-format #150

Open

cameronrutherford closed this Jun 5, 2024

pelesh assigned stonecoldhughes and unassigned ryandanehy Sep 20, 2024

pelesh added enhancement New feature or request testing labels Sep 20, 2024

pelesh reopened this Sep 20, 2024

pelesh marked this pull request as draft September 20, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Precommit addition to the repo #82

Precommit addition to the repo #82

ryandanehy commented Nov 27, 2023 •

edited by pelesh

Loading

pelesh commented Nov 29, 2023

ryandanehy commented Nov 29, 2023

ryandanehy commented Nov 29, 2023

pelesh commented Nov 30, 2023

cameronrutherford left a comment •

edited

Loading

cameronrutherford Nov 30, 2023

cameronrutherford Nov 30, 2023

ryandanehy Nov 30, 2023

cameronrutherford Nov 30, 2023

cameronrutherford Dec 12, 2023

cameronrutherford Nov 30, 2023

cameronrutherford Dec 12, 2023

ryandanehy commented Nov 30, 2023

cameronrutherford commented Nov 30, 2023

pelesh commented Nov 30, 2023

cameronrutherford commented Nov 30, 2023

pelesh left a comment

ryandanehy commented Dec 8, 2023

cameronrutherford left a comment

cameronrutherford Dec 12, 2023

cameronrutherford Dec 12, 2023

cameronrutherford Dec 12, 2023

cameronrutherford Dec 12, 2023

cameronrutherford Dec 12, 2023

pelesh commented Dec 28, 2023

ryandanehy commented Dec 29, 2023

cameronrutherford commented Jan 16, 2024

Precommit addition to the repo #82

Are you sure you want to change the base?

Precommit addition to the repo #82

Conversation

ryandanehy commented Nov 27, 2023 • edited by pelesh Loading

pelesh commented Nov 29, 2023

ryandanehy commented Nov 29, 2023

ryandanehy commented Nov 29, 2023

pelesh commented Nov 30, 2023

cameronrutherford left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryandanehy commented Nov 30, 2023

cameronrutherford commented Nov 30, 2023

pelesh commented Nov 30, 2023

cameronrutherford commented Nov 30, 2023

pelesh left a comment

Choose a reason for hiding this comment

ryandanehy commented Dec 8, 2023

cameronrutherford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pelesh commented Dec 28, 2023

ryandanehy commented Dec 29, 2023

cameronrutherford commented Jan 16, 2024

ryandanehy commented Nov 27, 2023 •

edited by pelesh

Loading

cameronrutherford left a comment •

edited

Loading