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.

Sign up for GitHub

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

Implement pre-commit hooks. #3

Merged
merged 5 commits into from
Apr 5, 2024
Merged

Implement pre-commit hooks. #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e6296b
IP-241 - Implement pre-commit hooks.
owenlittlejohns Mar 29, 2024
c75775b
IP-241 - Use black across all code in repository.
owenlittlejohns Apr 4, 2024
308e8ff
IP-241 - Add .git-blame-ignore-revs.
owenlittlejohns Apr 4, 2024
10c452b
IP-241 - Increment service version for black formatting.
owenlittlejohns Apr 5, 2024
54820bf
Type / preserve RGBA array format / fix line too long
flamingbear Apr 5, 2024
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
5 changes: 5 additions & 0 deletions .git-blame-ignore-revs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# For more information, see:
# https://docs.github.com/en/repositories/working-with-files/using-files/viewing-a-file#ignore-commits-in-the-blame-view

# Black code formatting of entire repository
c75775b10b19a361fd8ecfdaccc2bb2f820ea115
Copy link
Member Author

Choose a reason for hiding this comment

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

Please check this commit hash matches the one for that implements black across the whole repo. I'm super paranoid I've got the wrong one!

37 changes: 19 additions & 18 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, shoot. You already did this! Gaaah. Are you fine with just black instead of darker? Do you have a preference. Sorry to miss this!

Copy link
Member

Choose a reason for hiding this comment

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

From what I can tell when black-jupyter pre-commit runs, it will clean up an entire file. That could lead to a lot of git blames on unrelated changes.

I'm all for blacking these up, but should we consider including a full black commit on each repo that is excluded from git blame?

Something like this https://black.readthedocs.io/en/stable/guides/introducing_black_to_your_project.html#avoiding-ruining-git-blame

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this approach - thanks for talking through it with me on Tuesday. Here's my plan:

  • I'll preemptively squash the current commits into a single one (because Squash and Merge isn't an option here). I know it's a bit bleugh to force push once stuff is in PR, but I think this time makes sense, where the later commits can't be squashed.
  • I'll use black over all Python files (including test files). This will be it's own commit, for clarity.
  • Then I'll add the secondary commit to ignore the black commit when using git blame (this is really cool by the way).
  • Then I'll push up and we'll be set. 🤞

Other things:

  • I feel like if there is any change in the service code, we probably should have an associated patch release, because even though the code should be unchanged functionally, I feel like we need to test this in isolation. Does this sound reasonable, or am I just being paranoid?
  • Later work should, in the near future, add these checks to CI/CD, as we discussed at some point in the last couple of days.
  • I'll try to prioritise HyBIG, because I think that's the repository where you'll be working next.

Does that all sound good?

Copy link
Member

Choose a reason for hiding this comment

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

That all sounds great to me. I was trying to get the black checks into the workflows but with internet and planning needs, I couldn't do it. Def want to do that next. I may sneak it in next time I work on any of these projects.

I think that plan is fine as long as you don't squash merge this PR.

Release question? IDK either way, I would do it because I'm lazy and there are no code changes, but I don't mind having one.

Actually we're changing code, even if it's just black formatting. So a patch is warranted.

Copy link
Member

Choose a reason for hiding this comment

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

We talked about this later and you did a patch release.

repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-yaml
- id: check-added-large-files
- repo: https://github.com/akaihola/darker
rev: 1.7.2
hooks:
- id: darker
args:
- --isort
- --skip-string-normalization
additional_dependencies:
- isort~=5.13.1
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.2.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-json
- id: check-yaml
- id: check-added-large-files
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.3.4
hooks:
- id: ruff
args: ["--fix", "--show-fixes"]
- repo: https://github.com/psf/black-pre-commit-mirror
rev: 24.3.0
hooks:
- id: black-jupyter
args: ["--skip-string-normalization"]
language_version: python3.11
Copy link
Member

Choose a reason for hiding this comment

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

You don't want to commit?

Copy link
Member

Choose a reason for hiding this comment

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

You didn't get to see this.

6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## v1.0.1
### 2024-04-05

This version of HyBIG updates the repository to use `black` code formatting
throughout. There should be no functional change to the service.

## v1.0.0
### 2024-01-22
This version of the Harmony Browse Image Generator (HyBIG) contains all
Expand Down
33 changes: 33 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,39 @@ newest relate of the code (starting at the top of the file).
## vX.Y.Z
```

### pre-commit hooks:

This repository uses [pre-commit](https://pre-commit.com/) to enable pre-commit
checking the repository for some coding standard best practices. These include:

* Removing trailing whitespaces.
* Removing blank lines at the end of a file.
* JSON files have valid formats.
* [ruff](https://github.com/astral-sh/ruff) Python linting checks.
* [black](https://black.readthedocs.io/en/stable/index.html) Python code
formatting checks.

To enable these checks:

```bash
# Install pre-commit Python package as part of test requirements:
pip install -r tests/pip_test_requirements.txt

# Install the git hook scripts:
pre-commit install

# (Optional) Run against all files:
pre-commit run --all-files
```

When you try to make a new commit locally, `pre-commit` will automatically run.
If any of the hooks detect non-compliance (e.g., trailing whitespace), that
hook will state it failed, and also try to fix the issue. You will need to
review and `git add` the changes before you can make a commit.

It is planned to implement additional hooks, possibly including tools such as
`mypy`.

## Releasing a new version of the service:

Once a new Docker image has been published with a new semantic version tag,
Expand Down
2 changes: 1 addition & 1 deletion docker/service_version.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.0.0
1.0.1
44 changes: 24 additions & 20 deletions docs/HyBIG-Example-Usage.ipynb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"\n",
"# creates an output directory for the downloaded files\n",
"from pathlib import Path\n",
"\n",
"output_dir = Path('./hybig-output')\n",
"Path.mkdir(output_dir, exist_ok=True)"
]
Expand All @@ -79,12 +80,11 @@
"metadata": {},
"outputs": [],
"source": [
"\n",
"aster_collection = Collection(id='C1256584478-EEDTEST')\n",
"aster_granule = 'G1256584570-EEDTEST'\n",
"\n",
"measures_collection = Collection(id='C1258119317-EEDTEST')\n",
"measures_granule = 'G1258119387-EEDTEST'\n",
"measures_granule = 'G1258119387-EEDTEST'\n",
"\n",
"harmony_client = Client(env=Environment.UAT)"
]
Expand Down Expand Up @@ -118,9 +118,7 @@
"\n",
"# Specify a request to create a browse image from an MEaSUREs granule:\n",
"measures_request = Request(\n",
" collection=measures_collection,\n",
" granule_id=measures_granule,\n",
" format=image_format\n",
" collection=measures_collection, granule_id=measures_granule, format=image_format\n",
")\n",
"\n",
"# Submit the request and wait for it to complete:\n",
Expand All @@ -132,8 +130,9 @@
"Path.mkdir(example1_output_dir, exist_ok=True)\n",
"downloaded_outputs = [\n",
" file_future.result()\n",
" for file_future\n",
" in harmony_client.download_all(measures_job_id, overwrite=True, directory=example1_output_dir)\n",
" for file_future in harmony_client.download_all(\n",
" measures_job_id, overwrite=True, directory=example1_output_dir\n",
" )\n",
"]"
]
},
Expand Down Expand Up @@ -234,7 +233,7 @@
" collection=aster_collection,\n",
" granule_id=aster_granule,\n",
" scale_extent=scale_extent,\n",
" format='image/jpeg'\n",
" format='image/jpeg',\n",
")\n",
"\n",
"# Submit the request and wait for it to complete:\n",
Expand All @@ -246,8 +245,9 @@
"Path.mkdir(example2_output_dir, exist_ok=True)\n",
"downloaded_extent_outputs = [\n",
" file_future.result()\n",
" for file_future\n",
" in harmony_client.download_all(extent_job_id, overwrite=True, directory=example2_output_dir)\n",
" for file_future in harmony_client.download_all(\n",
" extent_job_id, overwrite=True, directory=example2_output_dir\n",
" )\n",
"]"
]
},
Expand Down Expand Up @@ -312,7 +312,7 @@
" collection=measures_collection,\n",
" granule_id=measures_granule,\n",
" scale_size=scale_sizes,\n",
" format='image/png'\n",
" format='image/png',\n",
")\n",
"\n",
"# Submit the request and wait for it to complete:\n",
Expand All @@ -324,8 +324,9 @@
"Path.mkdir(example3_output_dir, exist_ok=True)\n",
"downloaded_scale_size_outputs = [\n",
" file_future.result()\n",
" for file_future\n",
" in harmony_client.download_all(scale_size_job_id, overwrite=True, directory=example3_output_dir)\n",
" for file_future in harmony_client.download_all(\n",
" scale_size_job_id, overwrite=True, directory=example3_output_dir\n",
" )\n",
"]"
]
},
Expand Down Expand Up @@ -366,8 +367,9 @@
"dimensions_request = Request(\n",
" collection=measures_collection,\n",
" granule_id=measures_granule,\n",
" height=180, width=180,\n",
" format='image/png'\n",
" height=180,\n",
" width=180,\n",
" format='image/png',\n",
")\n",
"\n",
"# Submit the request and wait for it to complete:\n",
Expand All @@ -379,8 +381,9 @@
"Path.mkdir(example4_output_dir, exist_ok=True)\n",
"downloaded_dimensions_outputs = [\n",
" file_future.result()\n",
" for file_future\n",
" in harmony_client.download_all(dimensions_job_id, overwrite=True, directory=example4_output_dir)\n",
" for file_future in harmony_client.download_all(\n",
" dimensions_job_id, overwrite=True, directory=example4_output_dir\n",
" )\n",
"]"
]
},
Expand Down Expand Up @@ -432,7 +435,7 @@
" granule_id=measures_granule,\n",
" scale_extent=iceland_extent,\n",
" scale_size=iceland_scale_size,\n",
" format='image/png'\n",
" format='image/png',\n",
")\n",
"\n",
"# Submit the request and wait for it to complete:\n",
Expand All @@ -444,8 +447,9 @@
"Path.mkdir(example5_output_dir, exist_ok=True)\n",
"downloaded_tiled_outputs = [\n",
" file_future.result()\n",
" for file_future\n",
" in harmony_client.download_all(tiled_job_id, overwrite=True, directory=example5_output_dir)\n",
" for file_future in harmony_client.download_all(\n",
" tiled_job_id, overwrite=True, directory=example5_output_dir\n",
" )\n",
"]"
]
},
Expand Down
10 changes: 6 additions & 4 deletions harmony_browse_image_generator/__main__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
""" Run the Harmony Browse Image Generator Adapter via the Harmony CLI. """

from argparse import ArgumentParser
from sys import argv

Expand All @@ -9,12 +10,13 @@


def main(arguments: list[str]):
""" Parse command line arguments and invoke the appropriate method to
respond to them
"""Parse command line arguments and invoke the appropriate method to
respond to them

"""
parser = ArgumentParser(prog=SERVICE_NAME,
description='Run Harmony Browse Image Generator.')
parser = ArgumentParser(
prog=SERVICE_NAME, description='Run Harmony Browse Image Generator.'
)

setup_cli(parser)
harmony_arguments, _ = parser.parse_known_args(arguments[1:])
Expand Down
Loading
Loading