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

Add pre-commit hooks and Github action for linting and formatting #168

Merged
merged 8 commits into from
Feb 22, 2024

Conversation

Wiebke
Copy link
Member

@Wiebke Wiebke commented Feb 21, 2024

This PR adds back the Github actions for linting and formatting that used to be part of .github/workflows/staging-deploy.yml but were removed in #158 and adds and applies corresponding pre-commit hooks and settings for black, isort, and flake8, following black's compatibility guidelines.

Additional Notes

The test-and-lint action is applied only on pull requests, not commits, and running the hooks thus can be omitted with the --no-verify flag for commits with work in progress.

To setup pre-commit hooks run pip install -r requirements-dev.txt, followed by pre-commit install. Aside from during commits, the hooks can also be run with pre-commit run --all-files.

Some selected lines have been marked with # noqa: to ignore flake8 errors. These are the dash component imports as well as the long lines with cmd specifications.

Additionally closes #167, as the sample data directory will be created if it doesn't exists when running python3 utils/download_sample_data.py

This omits the just deleted /data directory.

This file and corresponding variables no longer be used at all once mlexchange#160 is addressed.
@Wiebke Wiebke requested a review from taxe10 February 21, 2024 16:42
@Wiebke
Copy link
Member Author

Wiebke commented Feb 21, 2024

Questions

black has a default line length of 88 characters. It will not auto-format comments, docstrings, or strings. The previous Github action in this repo had flake8 check for a line length of 127 characters with the comment that is in line with the Github code editor. This means effectively since black runs before flake8 in the pre-commit hooks, flake8 will only complain about long lines with comments, docstrings, or strings. Is that sensible?

I looked at Tiled's pre-commit hooks (bluesky/tiled/blob/main/.pre-commit-config.yaml) as an example. There are a number not necessarily Python-related hooks that may be sensible to include as well, e.g. removing trailing white-space, uniform ending of files, warning about debug statements, etc. Should we also include these?

@dylanmcreynolds
Copy link
Member

There are a number not necessarily Python-related hooks that may be sensible to include as well, e.g. removing trailing white-space, uniform ending of files, warning about debug statements, etc. Should we also include these?

Yes. I think these are python, actually. flake8 certainly complains about trailing white-space and uniform ending of files. I don't klnow if this will warn of breakpoint() statement, but it sure would be nice.

Copy link
Member

@dylanmcreynolds dylanmcreynolds left a comment

Choose a reason for hiding this comment

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

Looks good. I added a couple of minor comments.

- name: Set up Python 3.9.16
uses: actions/setup-python@v3
with:
python-version: '3.9.16'
Copy link
Member

Choose a reason for hiding this comment

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

Should we use 3.11?

Copy link
Member Author

Choose a reason for hiding this comment

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

A higher version makes sense, yes.

However, noticed that our Docker image has python:3.9 as a base image as well and thus would like to address that in a separate PR.

requirements-dev.txt Outdated Show resolved Hide resolved
Copy link
Member

@dylanmcreynolds dylanmcreynolds left a comment

Choose a reason for hiding this comment

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

Looks good. I added a couple of minor comments.

Copy link
Member

@taxe10 taxe10 left a comment

Choose a reason for hiding this comment

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

All looks good to me!

for trimming trailing white spaces, fixing ends of files, and checking for broken symlinks, case and merge conflicts, valid python and yaml
@Wiebke Wiebke merged commit 26a8dcb into mlexchange:main Feb 22, 2024
2 checks passed
@Wiebke Wiebke deleted the test-and-lint-hooks-and-actions branch February 22, 2024 19:35
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.

Shoud the data/.tmp file be in github?
3 participants