Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .github/workflows/github-python-workflow.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# This workflow will install Python dependencies, run tests and lint with a single version of Python
# For more information see: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python

name: Python Workflow

on:
push:
branches: [main, develop]
pull_request:
branches: [main, develop]

jobs:
test:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
fetch-tags: true
- name: Set up Python 3.11
uses: actions/setup-python@v3
with:
python-version: "3.11"
- name: Install dependencies
run: |
pip install --upgrade pip setuptools && pip install .[dev]
- name: Run flake8 lint checks
run: |
# stop the build if there are Python syntax errors or undefined names
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics
# exit-zero treats all errors as warnings
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=120 --statistics
- name: Run black format checks
run: black --check --diff ogc example || true # For now pass even if files need formatting
- name: Run pytest with coverage
run: |
pytest -v --cov=ogc
7 changes: 7 additions & 0 deletions .pre-commit-config.yaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For my own curiosity more than anything else - what does this file affect? Does it create a pre-commit hook for commits that are done through the GitHub site, or is it something that's meant to take effect on developer machines?

Copy link
Copy Markdown
Contributor

@mpu-creare mpu-creare May 28, 2025

Choose a reason for hiding this comment

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

It's supposed to take effect on the developer's machine. This prevents you from committing code that hasn't been formatted using Black. Unfortunately, there have been cases where developers do not properly install this precommit hook, and so unformatted text does show up in the repo.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting - how does one install that hook? I don't see any directions in the readme, and git commands usually don't take yaml files, is there a tool we're supposed to use?

We could also flip a switch now, to make Black formatting mandatory for pull requests to be accepted: line 35

I'm personally not a fan of pre-commit hooks for a couple reasons:

  • I think the point where formatting matters is merge time. I personally like being able to commit stuff that doesn't meet any quality bar, as long as it's not on the develop or main branch, because it lets me use git as a simple backup mechanism. Developers can commit something that doesn't even have correct syntax, just to have a checkpoint at the end of the workday or whatever.
  • It requires a manual action by every developer on every checkout - they have to know to install the pre-commit hook. In contrast, if we lean on CI checks, it's all automated and nobody has to remember anything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The hook "used" to get automatically installed with setup.py. See here: https://github.com/creare-com/ogc/blob/main/setup.py#L66C7-L66C25 and here: https://github.com/creare-com/ogc/blob/main/setup.py#L102

With Python's move to a pyproject.toml file, I'm not sure how this works anymore.

The main advantages of this approach is that the automatic formatting makes the total codebase smaller, and you don't have to look at all the changes to to formatting when you're reviewing a merge request. In theory it's also automatic with no effort on the developer's part... in practice the hook seems to misbehave. Personally, I'm a fan of doing both the CI check and the hook.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Personally, I'm a fan of doing both the CI check and the hook.

I can totally get behind that! Seems like one of those situations where the redundancy is helpful.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Setup.py still gets called with pyproject.toml present. However our PostDevelopCommand does not. In order to get the pre commit install you now need to run python setup.py develop directly. I can add a note on this or come up with a workaround.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we provide a setup.sh file in our python project template to install precommit hooks now... that was the best workaround we came up with.

Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
repos:
- repo: https://github.com/ambv/black
rev: stable
hooks:
- id: black
language_version: python3
files: (^ogc|^example)
1 change: 1 addition & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ dev = [
# FORMATTING
"pre_commit",
"black",
"flake8",
]

[tool.setuptools.packages.find]
Expand Down