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

Clarify ruff functionality. #403

Merged
merged 3 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 3 additions & 2 deletions copier.yml
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,11 @@ python_versions:
enforce_style:
help: What tooling set would you like to use to enforce code style? Use SPACE to highlight all that apply.
type: str
default: [ruff]
default: [ruff_lint, ruff_format]
multiselect: true
choices:
ruff: ruff
ruff linting: ruff_lint
ruff auto-formatter: ruff_format
pylint: pylint
black: black
isort: isort
Expand Down
3 changes: 2 additions & 1 deletion docs/practices/ci_precommit.rst
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ For each pull request, our ``pre-commit.ci`` workflow will:
* Analyze the src code style and report code that doesn't adhere, using
the options you selected for your tooling set:

* "ruff": checks for linting rules, sorts imports, auto-formats code
* "ruff linting": checks for linting rules
* "ruff auto-formatter": sorts imports, auto-formats code
* "pylint": checks for compliance with pylint rules
* "black": auto-formats code (including notebooks)
* "isort": Sort imports using ``isort``
Expand Down
6 changes: 5 additions & 1 deletion docs/practices/precommit.rst
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,11 @@ of these that are not useful for your project.
- Runs black to enforce a particular code style on .py, .pyi and .ipynb files.
One can add more file extensions by editing this hook on ``.pre-commit-config.yaml``
directly. *(Optionally installed when project is created.)*
* - Format and lint code using ruff
* - Lint code using ruff
- Runs ruff to enforce a particular code style on .py, .pyi and .ipynb files.
One can add more file extensions by editing this hook on ``.pre-commit-config.yaml``
directly. *(Optionally installed when project is created.)*
* - Format code using ruff
- Runs ruff to enforce a particular code style on .py, .pyi and .ipynb files.
One can add more file extensions by editing this hook on ``.pre-commit-config.yaml``
directly. *(Optionally installed when project is created.)*
Expand Down
2 changes: 1 addition & 1 deletion docs/source/new_project.rst
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ questions:
We provide several compatible options for linters and autoformatters.
Choosing a formatter or linter will include it as a project dependency and include it in the
:doc:`pre-commit <../practices/precommit>` hooks.
Defaults to ``ruff`` during simple installation.
Defaults to ``ruff`` (linting and autoformatting) during simple installation.
* - *How would you like to receive workflow failure notifications?*
- See :doc:`/practices/ci_testing`.
Some github workflows are not loud about their failures, so we have some configuration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,13 @@ jobs:
pylint -rn -sn --recursive=y ./benchmarks --rcfile=./tests/.pylintrc
{%- endif -%}
{%- endif -%}
{%- if 'ruff' in enforce_style %}
- name: Analyze code with ruff
{%- if 'ruff_lint' in enforce_style %}
- name: Analyze code with ruff formatter
run: |
ruff format --check
{%- endif -%}
{%- if 'ruff_format' in enforce_style %}
- name: Analyze code with ruff linter
run: |
ruff check
{%- endif -%}
15 changes: 13 additions & 2 deletions python-project-template/.pre-commit-config.yaml.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,25 @@ repos:
# https://pre-commit.com/#top_level-default_language_version
language_version: python3.10
{%- endif %}
{%- if 'ruff' in enforce_style %}
{%- if 'ruff_lint' in enforce_style %}
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.3
hooks:
- id: ruff
name: Format and lint code using ruff
name: Lint code using ruff
types_or: [ python, pyi, jupyter ]
args: ["check"]
{%- endif %}
{%- if 'ruff_format' in enforce_style %}
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.3
hooks:
- id: ruff
name: Format code using ruff
types_or: [ python, pyi, jupyter ]
args: ["format", "--check"]
{%- endif %}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the hook's readme proposes to put hooks in this order. If we do this, the linter would fail for some bad formatting (for example long lines), before formatter runs. I suggest to swap the order, so formatter runs first and fixes some linting issues

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree with the ruff folks on this one!

Since the pre-commit is mostly about warning, and not about fixing the changes, I'm a fan of the lint-first-then-format ordering. This would also alert sooner about "code smell" type problems, before alerting about more minor formatting issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

My point is that we can automatically fix formatting with the first hook, instead of fail

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll move this discussion to a new issue. Thank you!

{%- if mypy_type_checking != 'none' %}
# Analyze type hints and report errors.
Expand Down
2 changes: 1 addition & 1 deletion python-project-template/pyproject.toml.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ dev = [
{%- if 'black' in enforce_style %}
"black", # Used for static linting of files
{%- endif %}
{%- if 'ruff' in enforce_style %}
{%- if 'ruff_lint' in enforce_style or 'ruff_format' in enforce_style %}
"ruff", # Used for static linting of files
{%- endif %}
{%- if mypy_type_checking != 'none' %}
Expand Down
70 changes: 48 additions & 22 deletions tests/test_package_creation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,24 @@
import pytest_copie
import subprocess


def successfully_created_project(result):
"""Basic assertions that indicate the copier was able to create a project"""
return result.exit_code == 0 and result.exception is None and result.project_dir.is_dir()


def directory_structure_is_correct(result, package_name = "example_package"):
def directory_structure_is_correct(result, package_name="example_package"):
"""Test to ensure that the default directory structure ws created correctly"""
return (result.project_dir / f"src/{package_name}").is_dir() and (result.project_dir / f"tests/{package_name}").is_dir()
return (result.project_dir / f"src/{package_name}").is_dir() and (
result.project_dir / f"tests/{package_name}"
).is_dir()


def black_runs_successfully(result):
"""Test to ensure that the black linter runs successfully on the project"""
# run black with `--check` to look for lint errors, but don't fix them.
black_results = subprocess.run(
["python", "-m", "black", "--check", (result.project_dir / "src")],
cwd=result.project_dir
["python", "-m", "black", "--check", (result.project_dir / "src")], cwd=result.project_dir
)

return black_results.returncode == 0
Expand All @@ -27,27 +29,21 @@ def pylint_runs_successfully(result):
"""Test to ensure that the pylint linter runs successfully on the project"""
# run pylint to ensure that the hydrated files are linted correctly
pylint_results = subprocess.run(
["python", "-m", "pylint",
"--recursive=y",
"--rcfile=./src/.pylintrc",
(result.project_dir / "src")],
cwd=result.project_dir
["python", "-m", "pylint", "--recursive=y", "--rcfile=./src/.pylintrc", (result.project_dir / "src")],
cwd=result.project_dir,
)

return pylint_results.returncode == 0


def unit_tests_in_project_run_successfully(result, package_name = "example_package"):
def unit_tests_in_project_run_successfully(result, package_name="example_package"):
"""Test to ensure that the unit tests run successfully on the project

!!! NOTE - This doesn't currently work because we need to `pip install` the hydrated
project before running the tests. And we don't have a way to create a temporary
virtual environment for the project.
"""
pytest_results = subprocess.run(
["python", "-m", "pytest"],
cwd=result.project_dir
)
pytest_results = subprocess.run(["python", "-m", "pytest"], cwd=result.project_dir)

return pytest_results.returncode == 0

Expand All @@ -73,10 +69,10 @@ def test_all_defaults(copie):
# check to see if the README file was hydrated with copier answers.
found_line = False
with open(result.project_dir / "README.md") as f:
for line in f:
if "example_project" in line:
found_line = True
break
for line in f:
if "example_project" in line:
found_line = True
break
assert found_line


Expand Down Expand Up @@ -107,10 +103,40 @@ def test_use_black_and_no_example_modules(copie):
# check to see if the pyproject.toml file has the expected dependencies
found_line = False
with open(result.project_dir / "pyproject.toml") as f:
for line in f:
if "\"black\", # Used for static linting of files" in line:
found_line = True
break
for line in f:
if '"black", # Used for static linting of files' in line:
found_line = True
break
assert found_line

assert black_runs_successfully(result)


@pytest.mark.parametrize(
"enforce_style",
[
[],
["ruff_lint"],
["ruff_format"],
["ruff_lint", "pylint"],
["ruff_format", "black"],
["black", "pylint", "isort"],
["ruff_lint", "ruff_format"],
["black", "pylint", "isort", "ruff_lint", "ruff_format"],
],
)
def test_code_style_combinations(copie, enforce_style):
"""Test that various combinations of code style enforcement will
still result in a valid project being created."""

# provide a dictionary of the non-default answers to use
extra_answers = {
"enforce_style": enforce_style,
}

# run copier to hydrate a temporary project
result = copie.copy(extra_answers=extra_answers)
assert successfully_created_project(result)
assert directory_structure_is_correct(result)
# black would still run successfully.
assert black_runs_successfully(result)