Skip to content
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

github: formatting: add check on push and pull-request #810

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kkanas
Copy link
Contributor

@kkanas kkanas commented Nov 15, 2024

Add pre-commit to allow local check before commit. This requires installing pre-commit on local machine. Flow to install and check/debug:

pip install pre-commit
pre-commit install
# run pre-commit on single file file_to_check.txt pre-commit run --verbose --files file_to_check.txt # run pre-commit on all files
pre-commit run --verbose --all-files

Rules for pre-commit are in .pre-commit-config.yaml: Currently rules are:

check-added-large-files:
Will stop commit if large files to the repo
mixed-line-ending:
args: ['--fix', 'no']
Reports rule and stops on when file has mixed line endings
will run on all files in repository excluding files ending with .cmd
mixed-line-ending:
args: ['--fix', 'crlf']
files: '.cmd$'
Will automatically correct lf to crlf on files ending with .cmd

To uphold rules on server new github workflow is added that will execute pre-commit run --all-files

Lastly I changed .gitattributes to also force crlf in cmd files.

Description

Describe your changes in as much detail as possible. Provide a link/reference to the issue solved with this request if any.

Checklist

  • I have read the contribution guidelines.
  • All unit tests are passing.
  • I have merged the latest main branch prior to this PR submission.
  • I submitted this PR against the main branch.

Add pre-commit to allow local check before commit.
This requires installing pre-commit on local machine.
Flow to install and check/debug:

pip install pre-commit
pre-commit install
\# run pre-commit on single file file_to_check.txt
pre-commit run --verbose --files file_to_check.txt
\#  run pre-commit on all files
pre-commit run --verbose --all-files

Rules for pre-commit are in .pre-commit-config.yaml:
Currently rules are:

check-added-large-files:
    Will stop commit if large files to the repo
mixed-line-ending:
    args: ['--fix', 'no']
    Reports rule and stops on when file has mixed line endings
    will run on all files in repository excluding files ending with .cmd
mixed-line-ending:
      args: ['--fix', 'crlf']
      files: '\.cmd$'
      Will *automatically* correct lf to crlf on files ending with .cmd

To uphold rules on server new github workflow is added that will execute
pre-commit run --all-files

Lastly I changed .gitattributes to also force crlf in cmd files.

Signed-off-by: Krzysztof Kanas <[email protected]>
@kkanas kkanas requested a review from a team as a code owner November 15, 2024 19:59
* text=auto
*.cmd text eol=crlf
Copy link
Contributor

@MariusNi MariusNi Nov 15, 2024

Choose a reason for hiding this comment

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

cmd text eol=crlf

nit: line 3 is blank

)$
- id: mixed-line-ending
args: ['--fix', 'crlf']
files: '\.cmd$'
Copy link
Contributor

Choose a reason for hiding this comment

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

files: '.cmd$'

Same as the other nit: line 17 is blank

args: ['--fix', 'no']
exclude: |
(?x)(
.*\.cmd
Copy link
Contributor

Choose a reason for hiding this comment

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

.*.cmd

We also have .md, .jpg, .png,, etc. that need to be excluded

)$
- id: mixed-line-ending
args: ['--fix', 'crlf']
files: '\.cmd$'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we achieve the same all via .gitattributes? We can add there exclusions, default conversion, etc.

* text=auto
Copy link
Contributor

@MariusNi MariusNi Nov 15, 2024

Choose a reason for hiding this comment

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

Can we add here more, like for example:

# Set the default behavior to automatically normalize line endings.
* text=auto

# Explicitly declare text files you want to always be normalized and converted to LF upon checkout.
*.c text eol=lf
*.cpp text eol=lf
*.h text eol=lf
*.txt text eol=lf
*.json text eol=lf
*.yml text eol=lf
*.md text eol=lf

# Denote all files that are truly binary and should not be modified.
*.png binary
*.jpg binary

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.

2 participants