Skip to content
This repository was archived by the owner on Nov 11, 2025. It is now read-only.

Conversation

@michalslowikowski00
Copy link
Contributor

@michalslowikowski00 michalslowikowski00 commented Oct 9, 2020

Hello.

I would like to add pre-commit to the project.
IMHO this is a great and helpful tool for checking code before committing.
I have add few checks.
You can read about pre-commit here:
https://pre-commit.com/#install

What do you think about it?

PR contains:

  • pre-commit setup
  • pre-commit checks: black, isort, flake8, pydocstyle

# See the License for the specific language governing permissions and
# limitations under the License.

repos:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
repos:
---
default_stages: [commit, push]
default_language_version:
# force all unspecified python hooks to run python3
python: python3
minimum_pre_commit_version: "1.20.0"
repos:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool. I wasn't sure which python do we want to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we leave this hook in this PR?

- repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v2.3.0
    hooks:
      - id: check-yaml
      - id: end-of-file-fixer
      - id: trailing-whitespace

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable to me but as mentioned in https://github.com/apache/kibble/pull/44#pullrequestreview-505801054 it would be good to run pre-commit run --all-files to check all files

Comment on lines 25 to 50
- repo: https://github.com/psf/black
rev: 19.3b0
hooks:
- id: black
name: Black
types: [python]
- repo: https://github.com/timothycrosley/isort
rev: 5.5.4
hooks:
- id: isort
name: Isort
types: [python]
- repo: https://gitlab.com/pycqa/flake8
rev: 3.8.4
hooks:
- id: flake8
name: Flake8
types: [python]
- repo: https://github.com/pycqa/pydocstyle
rev: 5.1.1
hooks:
- id: pydocstyle
name: Pydocstyle
types: [python]
args:
- --convention=pep257
Copy link
Member

Choose a reason for hiding this comment

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

I would be in favor of introducing them in follow up PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I will move these hooks to separate PR.

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

Overall it looks good to me. But introducing pre-commit without actual changes (unless there are none to be applied) to files gives us nothing in my opinion. I would be in favor of introducing a simple Github Action CI that would run pre-commit.

This should do:

name: CI
on:
  - push
  - pull_request

jobs:
  statics:
    name: Static Checks
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v2
      - uses: actions/setup-python@v1
      - uses: pre-commit/action@v1.0.1

In this way if CI fails we will need to fix some issue and everyone will be able to see how pre-commit works and what problems are being detected.

@sharanf
Copy link
Contributor

sharanf commented Oct 10, 2020

@michalslowikowski00 @turbaszek thanks for helping move this initiative forward. I have highlighted that this work is now in progress and if anyone wants to contribute to this effort then they can join this conversation.

@sharanf
Copy link
Contributor

sharanf commented Oct 10, 2020

I will merge what has been done far.

@sharanf sharanf merged commit 02cc3fb into apache:master Oct 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants