Add a basic pre-commit lint and autoformat by adehad · Pull Request #448 · geldata/gel-python

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad · 2023-07-01T22:50:38Z

Following up on: #438

Summary of changes

a pre-commit config file has been added with some simple rules
Errors picked up during the linting have been addressed and added as separate commits for clarity

NOTE

Target branch

~~This is targetting the drop-py37 branch. As it was assumed this PR would be merged after #435 .~~

Running the lint

The assumption is that pre-commit.ci GitHub integration is going to be setup for this project and therefore a dedicate lint GHA has NOT been setup, although it may look like:

---
name: Linton:
push:
branches:
- master
- cipull_request:
branches:
- masterworkflow_dispatch:
inputs: {}jobs:
test:
runs-on: ubuntu-latestdefaults:
run:
shell: bashenv:
PIP_DISABLE_PIP_VERSION_CHECK: 1steps:
- uses: actions/checkout@v3with:
fetch-depth: 1submodules: true
- name: pre-commitrun: | python -m pip install pre-commit python -m pre_commit run --all-files

gel-data-cla · 2023-07-01T22:50:41Z

All commit authors signed the Contributor License Agreement.

adehad · 2023-07-01T22:57:33Z

@fantix Might be an issue that tests aren't running on this PR yet (due to it not targetting the master branch).
Happy to wait for #435 to be merged first

chrisemke

Consideration should be given to using the following hooks to enable it in the future:
id: blacken-docs (https://github.com/asottile/blacken-docs)
id: insert-license (https://github.com/Lucas-C/pre-commit-hooks)
id: conventional-pre-commit (https://github.com/compilerla/conventional-pre-commit)

Also re-enable mypy is very important for the future

chrisemke · 2023-07-08T21:30:20Z

+ - id: check-yaml
+ files: .*\.(yaml|yml)$


Suggested change
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-yaml
files: .*\.(yaml|yml)$
- id: check-added-large-files
- id: check-toml

chrisemke · 2023-07-08T21:32:02Z

+ - repo: https://github.com/psf/black
+ rev: 23.3.0
+ hooks:
+ - id: black


Add the name to make it more readable and beautiful when running
Suggested change
- id: black
- id: black
name: Black (Python formatter)

chrisemke · 2023-07-08T21:32:48Z

+ - repo: https://github.com/charliermarsh/ruff-pre-commit
+ rev: "v0.0.275"
+ hooks:
+ - id: ruff


Add the name to make it more readable and beautiful when running
Suggested change
- id: ruff
- id: ruff
name: Ruff (Python linter)

chrisemke · 2023-07-08T21:56:54Z

Another thing that would be really cool would be an edgedb hook to (but not limited to) verifying or formatting the .esdl and .edgeql files

adehad · 2023-07-11T13:40:07Z

@fantix feel free commit any of the suggestions.
(Personally I don't think the name addition to the pre-commit config adds much, I've mainly used it when I am running the same tool multiple times but with different configurations)

fantix · 2023-09-14T21:57:14Z

Sorry, I didn't mean to close this PR.

adehad · 2023-09-18T11:08:18Z

@fantix do let me know if you would like me to target the main branch instead of this drop-py37 branch

fantix · 2023-09-18T14:51:12Z

Yes, please! Let's retarget and rebase to the master branch, and get this merged 🙏

adehad · 2023-09-18T16:27:49Z

@fantix should be ready for review again now

adehad · 2023-09-29T11:33:11Z

Didn't realise there were merge conflicts, should now be resolved and ready for review @fantix

adehad · 2023-10-05T11:08:01Z

(bump) @fantix

adehad · 2024-01-15T22:34:51Z

@fantix have just rebased now

fantix · 2024-01-15T22:41:04Z

Thanks for the updates! I talked to the team and decided we don't want a pre-commit hook yet. However, I think the formatted code could stay (with a few nitpicking, for which I'll add comments on this PR) and we can probably have a test that runs the linter and formatter for checking.

adehad · 2024-01-15T22:48:50Z

and we can probably have a test that runs the linter and formatter for checking.

Yep, we do this too rather that as a pre-commit hook itself. I can recommend using the pre-commit.ci github action/app as this automates the process of checking PRs

adehad · 2025-08-22T22:51:45Z

@msullivan any contributions accepted in this vein or shall I delete the branch on my end as well?

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

adehad mentioned this pull request Jul 1, 2023
add a basic pre-commit config adehad/edgedb-python#1
Closed

chrisemke reviewed Jul 8, 2023
View reviewed changes

fantix deleted the branch geldata:masterSeptember 14, 2023 20:04

fantix closed this Sep 14, 2023

fantix reopened this Sep 14, 2023

adehad force-pushed the chore/pre-commit branch from 0972123 to 6d46f3bCompare September 18, 2023 16:18

adehad changed the base branch from drop-py37 to masterSeptember 18, 2023 16:25

adehad force-pushed the chore/pre-commit branch from e02a529 to dbb3b3eCompare September 29, 2023 11:32

adehad added 7 commits January 15, 2024 22:26

add a basic pre-commit config
d1dce3f

use 79 for line length
30b1059

run autformatter and linters
bb1543d

fix codespell errors
ea80851

fix yamllint errors
6663122

fix E402: Module level import not at top of file
b40282f

fix E721: use isinstance when comparing types
543ae15

adehad force-pushed the chore/pre-commit branch from 2ea2509 to 543ae15Compare January 15, 2024 22:31

apply unsafe fixes: ruff . --fix --unsafe-fixes
f3b0c7b

msullivan closed this Aug 22, 2025

Uh oh!

Conversation

adehad commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

NOTE

Target branch

Running the lint

Uh oh!

gel-data-claBot commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adehad commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisemke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisemkeJul 8, 2023

Choose a reason for hiding this comment

Uh oh!

chrisemkeJul 8, 2023

Choose a reason for hiding this comment

Uh oh!

chrisemkeJul 8, 2023

Choose a reason for hiding this comment

Uh oh!

chrisemke commented Jul 8, 2023

Uh oh!

adehad commented Jul 11, 2023

Uh oh!

fantix commented Sep 14, 2023

Uh oh!

adehad commented Sep 18, 2023

Uh oh!

fantix commented Sep 18, 2023

Uh oh!

adehad commented Sep 18, 2023

Uh oh!

adehad commented Sep 29, 2023

Uh oh!

adehad commented Oct 5, 2023

Uh oh!

adehad commented Jan 15, 2024

Uh oh!

fantix commented Jan 15, 2024

Uh oh!

adehad commented Jan 15, 2024

Uh oh!

adehad commented Aug 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

adehad commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

NOTE

Target branch

Running the lint

Uh oh!

gel-data-claBot commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adehad commented Jul 1, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisemke left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisemkeJul 8, 2023

adehad commented Jul 1, 2023•
edited
Loading

gel-data-claBot commented Jul 1, 2023•
edited
Loading

adehad commented Jul 1, 2023•
edited
Loading

chrisemke left a comment •
edited
Loading

adehad commented Jul 1, 2023•
edited
Loading

gel-data-claBot commented Jul 1, 2023•
edited
Loading

adehad commented Jul 1, 2023•
edited
Loading

chrisemke left a comment •
edited
Loading

adehad commented Jul 1, 2023•
edited
Loading

gel-data-claBot commented Jul 1, 2023•
edited
Loading

adehad commented Jul 1, 2023•
edited
Loading

chrisemke left a comment •
edited
Loading