-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Precommit addition to the repo #82
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
--- | ||
BasedOnStyle: Google | ||
Language: Cpp | ||
AlignAfterOpenBracket: Align | ||
AlignConsecutiveAssignments: 'true' | ||
AlignConsecutiveDeclarations: 'true' | ||
AlignTrailingComments: 'true' | ||
AllowAllArgumentsOnNextLine: 'false' | ||
AllowAllParametersOfDeclarationOnNextLine: 'false' | ||
AlwaysBreakBeforeMultilineStrings: 'false' | ||
AlwaysBreakTemplateDeclarations: 'Yes' | ||
BinPackArguments: 'false' | ||
BinPackParameters: 'false' | ||
BreakBeforeBraces: Linux | ||
ColumnLimit: '100' | ||
ConstructorInitializerAllOnOneLineOrOnePerLine: 'true' | ||
FixNamespaceComments: 'true' | ||
IndentAccessModifiers: 'true' | ||
IndentCaseLabels: 'true' | ||
IndentWidth: '2' | ||
NamespaceIndentation: All | ||
PointerAlignment: Left | ||
SortIncludes: Never | ||
SpaceAfterCStyleCast: 'true' | ||
SpaceAfterLogicalNot: 'false' | ||
SpaceAfterTemplateKeyword: 'false' | ||
SpaceBeforeAssignmentOperators: 'true' | ||
SpaceBeforeCtorInitializerColon: 'true' | ||
SpaceBeforeInheritanceColon: 'true' | ||
SpaceBeforeParens: ControlStatements | ||
SpacesInParentheses: 'false' | ||
SpacesInSquareBrackets: 'false' | ||
Standard: Cpp11 | ||
TabWidth: '2' | ||
UseTab: Never | ||
|
||
--- | ||
Language: Json | ||
# Use 100 columns for JS. | ||
ColumnLimit: 100 | ||
--- | ||
Language: ObjC | ||
# Use 100 columns for C#. | ||
ColumnLimit: 100 | ||
... |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
name: documentation | ||
|
||
on: workflow_dispatch | ||
# on: [pull_request] | ||
|
||
|
||
permissions: | ||
contents: write | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
name: pre-commit GitHub Action | ||
|
||
# Won't run on develop/main directly | ||
on: | ||
workflow_dispatch: | ||
inputs: | ||
branch: | ||
description: 'The branch to build' | ||
required: true | ||
pull_request: | ||
|
||
jobs: | ||
pre-commit: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v3 | ||
with: | ||
ref: ${{ github.event.pull_request.head.ref }} | ||
- uses: actions/setup-python@v3 | ||
- uses: pre-commit/[email protected] | ||
# - uses: EndBug/[email protected] | ||
# # Only need to try and commit if the action failed | ||
# if: failure() | ||
# with: | ||
# fetch: false | ||
# committer_name: GitHub Actions | ||
# committer_email: [email protected] | ||
# message: Apply pre-commmit fixes | ||
Comment on lines
+21
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want pre-commit to automatically apply fixes, so lets comment this back in |
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
|
||
repos: | ||
- repo: https://github.com/cheshirekow/cmake-format-precommit | ||
rev: v0.6.13 | ||
hooks: | ||
- id: cmake-format | ||
- repo: https://github.com/pre-commit/mirrors-clang-format | ||
rev: v16.0.1 | ||
hooks: | ||
- id: clang-format | ||
- repo: https://github.com/pre-commit/pre-commit-hooks | ||
rev: v4.4.0 | ||
hooks: | ||
- id: forbid-new-submodules | ||
- id: end-of-file-fixer | ||
- id: check-yaml | ||
args: [--allow-multiple-documents] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,7 +53,42 @@ target_link_libraries(my_app PRIVATE ReSolve::ReSolve) | |
|
||
For all contributions to ReSolve please follow the [developer guidelines](CONTRIBUTING.md) | ||
|
||
### Pre-commit | ||
Precommit is a framework for mmangaing and maintain multi-lanagueg pre-commit hooks. https://pre-commit.com/ | ||
|
||
Precommit is auto applied through CI to every pull request and every commit to the said pull request. Precommit checks for valid yaml files, end of file spaces, Syntax of C codeand etc. To see specifically what formating precommit looks for please check out our .clang-format and .cmake-format filed as well as our .pre-commit-config.yaml which show the specific precommit hooks being applied. | ||
|
||
### Running Pre-commit locally | ||
To run pre-commit locally you first must install pre-commit and there are a couple ways to do this. | ||
|
||
#### Prerequiste to installing pre-commit | ||
It is recommended that you install pre-commit through python. Here we assume Python is aleadly installed if that is not the case please check out these [docs](https://wiki.python.org/moin/BeginnersGuide/Download) for how to install python. | ||
|
||
Next create a virtual environment with the following: | ||
```shell | ||
python -m venv /path/to/new/virtual/environment | ||
``` | ||
|
||
Activate the new environment: | ||
```shell | ||
source /path/to/new/virtual/environment/bin/activate | ||
``` | ||
|
||
Now that we are within a virtual environment we can install pre-commit via; | ||
```shell | ||
pip install pre-commit | ||
``` | ||
|
||
To install the hooks and have pre-commit run automatically before every commit run | ||
```shell | ||
pre-commit install --install-hooks | ||
``` | ||
|
||
If you want to manually run all pre-commit hooks on a repository, run | ||
```shell | ||
pre-commit run --all-files | ||
``` | ||
To learn more about pre-commit usuage check out https://pre-commit.com/#usage. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add some more information here about:
|
||
|
||
## Test and Deploy | ||
|
||
|
@@ -111,4 +146,3 @@ contributions to ReSolve must be made under the smae licensing terms. | |
**Please Note** If you are using ReSolve with any third party libraries linked | ||
in (e.g., KLU), be sure to review the respective license of the package as that | ||
license may have more restrictive terms than the ReSolve license. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just run on
pull_request
, doesn't that generate the desired behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to set it to manual so that people could start the workflow as needed. This is in reference to #82 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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