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

Use pre-commit as linter & Checker ✨ #345

Closed
wants to merge 2 commits into from
Closed

Use pre-commit as linter & Checker ✨ #345

wants to merge 2 commits into from

Conversation

yezz123
Copy link

@yezz123 yezz123 commented Oct 25, 2021

Based on this issue #344, I was thinking about why not use .pre-commit as a linter and checker.

Easy way to check code and fix any issue related to code style and code linting, about the rules I use only simple and most used rules for linting, SQLalchemy's team is free to change the rules or use another one for linting.

Note: This is just a simple enhancement to repository structure.

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

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

I'm personally -1 on some of these.

In particular I prefer to keep the configuration out of .pre-commit so that one is free to use the same tools with other runners.

python -m pip install pre-commit
- name: Test Project linting
run: |
pre-commit run --all-files
Copy link
Member

Choose a reason for hiding this comment

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

this removes the test run, so maybe we can have this together with the current version

rev: 5.6.4
hooks:
- id: isort
args: ["--profile", "black"]
Copy link
Member

Choose a reason for hiding this comment

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

we use zimports so -1 here

files: '\.py$'
exclude: docs/
args:
- --select=F403,F406,F821,T003
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes more sense to keep the configuration in setup.cfg

Copy link
Member

Choose a reason for hiding this comment

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

also tox is probably a better place for this

Copy link
Member

Choose a reason for hiding this comment

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

+1 to @CaselIT's suggestion on this. I generally like the idea of linting/checking on a pre-commit hook, but those checks will often need to be run on-demand during development -- and configuring them within pre-commit would require them to be duplicated in other config files.

- id: autoflake
files: '\.py$'
exclude: '^\..*'
args: ["--in-place"]
Copy link
Member

Choose a reason for hiding this comment

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

this seems to run autoflake. zimports does mostly the same

rev: 19.10b0
hooks:
- id: black
args: ["--target-version", "py37"]
Copy link
Member

Choose a reason for hiding this comment

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

please keep the current version. also the setting should be in pyproject.toml

hooks:
- id: check-merge-conflict
- id: check-added-large-files
- id: check-ast
Copy link
Member

Choose a reason for hiding this comment

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

not sure it's really needed. we use test for this

@CaselIT
Copy link
Member

CaselIT commented Oct 26, 2021

Thanks for the work. I've left a review.

It's probably best to wait for @zzzeek on this also so a single set of changes can be done

@CaselIT CaselIT requested a review from zzzeek October 26, 2021 14:34
@bourke
Copy link
Member

bourke commented Nov 30, 2021

Thanks for the contribution. As @CaselIT and @jvanasco mentioned above, this PR gives pre-commit more responsibility in our tooling than we're comfortable with — especially considering that we'd prefer to keep the configurations as consistent as we can across the sqlalchemy organisation. @zzzeek suggested that the configuration for dogpile.cache would be a good reference point.

@bourke bourke self-requested a review November 30, 2021 05:52
@bourke
Copy link
Member

bourke commented Dec 8, 2021

Hey there -- closing this for now. Let me know if you want to take it back up.

@bourke bourke closed this Dec 8, 2021
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.

4 participants