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

Add Ruff to pre-commit (before isort + flake8 to test it as a possible replacement) #490

Merged
merged 6 commits into from
Aug 30, 2023
Merged

Add Ruff to pre-commit (before isort + flake8 to test it as a possible replacement) #490

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e21db50
Switch from flake8 (and isort) to ruff
mfisher87 Aug 28, 2023
f015fdd
Autofix a manageable set of new rules
mfisher87 Aug 28, 2023
3aca73e
Specify target version
mfisher87 Aug 28, 2023
dbd131e
Enable and autofix B006
mfisher87 Aug 28, 2023
95f64ea
Restore flake8 and isort in a way that's compatible with ruff
mfisher87 Aug 29, 2023
52cb31b
Add context to isort skip comment
mfisher87 Aug 30, 2023
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: 0 additions & 5 deletions .flake8
View file
Open in desktop

This file was deleted.

19 changes: 7 additions & 12 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,21 @@ repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.4.0
hooks:
- id: trailing-whitespace
exclude: "^.*\\.patch$"
- id: check-ast
- id: trailing-whitespace
exclude: "^.*\\.patch$"
- id: check-ast

- repo: https://github.com/psf/black
rev: 23.7.0
hooks:
- id: black
language_version: python3

- repo: https://github.com/pycqa/flake8
rev: 6.1.0
- repo: https://github.com/astral-sh/ruff-pre-commit
rev: v0.0.286
hooks:
- id: flake8

- repo: https://github.com/pycqa/isort
rev: 5.12.0
hooks:
- id: isort
args: ["--profile", "black", "--filter-files"]
- id: ruff
args: ["--fix"]

- repo: https://github.com/pre-commit/mirrors-mypy
rev: v1.5.1
Expand Down
10 changes: 6 additions & 4 deletions conda_lock/conda_lock.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ def fn_to_dist_name(fn: str) -> str:
return fn


def make_lock_files( # noqa: C901
def make_lock_files(
*,
conda: PathLike,
src_files: List[pathlib.Path],
Expand Down Expand Up @@ -768,7 +768,7 @@ def create_lockfile_from_spec(
*,
conda: PathLike,
spec: LockSpecification,
platforms: List[str] = [],
platforms: Optional[List[str]] = None,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking out loud: This is probably safe as-is because this function would only be called one time per CLI invocation. But if that assumption changes, this use of a mutable default value could bite in the future.

lockfile_path: pathlib.Path,
update_spec: Optional[UpdateSpecification] = None,
metadata_choices: AbstractSet[MetadataOption] = frozenset(),
Expand All @@ -778,6 +778,8 @@ def create_lockfile_from_spec(
"""
Solve or update specification
"""
if platforms is None:
platforms = []
assert spec.virtual_package_repo is not None
virtual_package_channel = spec.virtual_package_repo.channel

Expand Down Expand Up @@ -943,10 +945,10 @@ def _render_lockfile_for_install(
if platform not in lock_content.metadata.platforms:
suggested_platforms_section = "platforms:\n- "
suggested_platforms_section += "\n- ".join(
[platform] + lock_content.metadata.platforms
[platform, *lock_content.metadata.platforms]
)
suggested_platform_args = "--platform=" + " --platform=".join(
[platform] + lock_content.metadata.platforms
[platform, *lock_content.metadata.platforms]
)
raise PlatformValidationError(
f"The lockfile {filename} does not contain a solution for the current "
Expand Down
2 changes: 1 addition & 1 deletion conda_lock/conda_solver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ def update_specs_for_arch(
proc = subprocess.run(
[
str(arg)
for arg in args + ["-p", prefix, "--json", "--dry-run", *to_update]
for arg in [*args, "-p", prefix, "--json", "--dry-run", *to_update]
],
env=conda_env_override(platform),
stdout=subprocess.PIPE,
Expand Down
4 changes: 3 additions & 1 deletion conda_lock/lockfile/v2prelim/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
GitMeta,
HashModel,
InputMeta,
LockMeta,
MetadataOption,
TimeMeta,
)
from conda_lock.lockfile.v1.models import LockedDependency as LockedDependencyV1
from conda_lock.lockfile.v1.models import Lockfile as LockfileV1
from conda_lock.lockfile.v1.models import LockMeta, MetadataOption, TimeMeta
from conda_lock.models import StrictModel


Expand Down
2 changes: 1 addition & 1 deletion conda_lock/models/channel.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def _detect_used_env_var(

if value.startswith("$"):
return value.lstrip("$").strip("{}")
for suffix in preferred_env_var_suffix + [""]:
for suffix in [*preferred_env_var_suffix, ""]:
candidates = {v: k for k, v in os.environ.items() if k.upper().endswith(suffix)}
# try first with a simple match
key = candidates.get(value)
Expand Down
4 changes: 2 additions & 2 deletions conda_lock/src_parser/meta_yaml.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def __init__( # type: ignore
__mod__ = __rmod__ = __pos__ = __neg__ = __call__ = \
__getitem__ = __lt__ = __le__ = __gt__ = __ge__ = \
__complex__ = __pow__ = __rpow__ = \
lambda self, *args, **kwargs: self._return_undefined(self._undefined_name) # noqa: E122
lambda self, *args, **kwargs: self._return_undefined(self._undefined_name)
# fmt: on

# Accessing an attribute of an Undefined variable
Expand All @@ -60,7 +60,7 @@ def __getattr__(self, k: str) -> "UndefinedNeverFail":

# Unlike the methods above, Python requires that these
# few methods must always return the correct type
__str__ = __repr__ = lambda self: self._return_value(str()) # type: ignore # noqa: E731
__str__ = __repr__ = lambda self: self._return_value(str()) # type: ignore
__unicode__ = lambda self: self._return_value("") # noqa: E731
__int__ = lambda self: self._return_value(0) # type: ignore # noqa: E731
__float__ = lambda self: self._return_value(0.0) # type: ignore # noqa: E731
Expand Down
6 changes: 0 additions & 6 deletions environments/dev-environment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,19 +8,13 @@ dependencies:
- check-manifest
- doctr
- filelock
- flake8
- flake8-builtins
- flake8-comprehensions
- flake8-mutable
- python-build
- freezegun
- isort
- mypy
- pre-commit
- pylint
- pytest
- pytest-cov
- pytest-flake8
- pytest-xdist
- pytest-timeout
- tomli
Expand Down
53 changes: 43 additions & 10 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,6 @@ exclude = [
"tests",
]

[tool.isort]
atomic = true
force_grid_wrap = 0
include_trailing_comma = true
lines_after_imports = 2
lines_between_types = 1
multi_line_output = 3
use_parentheses = true
known_first_party = "attr"

[tool.pytest.ini_options]
addopts = "-vrsx -n auto"
flake8-max-line-length = 105
Expand Down Expand Up @@ -157,3 +147,46 @@ platforms = ["linux-64", "osx-64", "osx-arm64", "win-64", "osx-arm64", "linux-aa
# This is necessary to pull in the lockfile/filelock dependency
# since we don't handle the optional dependency.
cachecontrol-with-filecache = ">=0.12.9"


[tool.ruff]
target-version = "py38"
ignore = [
"E501",
"F401",
"F403",
# Disabled during migration to Ruff:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many of these can be enabled and autofixed, but I felt it would be better to tackle it in a separate PR to keep a reasonable scope for this one :)

"A001",
"A002",
"A003",
"C401",
"C405",
"C408",
"C409",
"C413",
"C414",
"C416",
"RUF012",
"RUF015",
]
line-length = 89
select = [
"A", # flake8-builtins
# "B", # flake8-bugbear
"B006", # Do not use mutable data structures for argument defaults
Copy link
Contributor Author

Choose a reason for hiding this comment

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

flake8-mutable errors are caught by this rule. Looks to have detected something that was previously undetected, too.

"C4", # flake8-comprehensions
"C9", # mccabe
"E", # pycodestyle errors
"F", # pyflakes
"I", # isort
"RUF", # ruff rules
"W", # pycodestyle warnings
]

[tool.ruff.mccabe]
max-complexity = 18

[tool.ruff.isort]
lines-after-imports = 2
lines-between-types = 1
known-first-party = ["attr"]
7 changes: 0 additions & 7 deletions requirements-dev.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,12 @@
black
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If black is installed and run with pre-commit, maybe it doesn't need to also be here? Same for mypy?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still useful to have in the dev environment. If you're running VS Code, then black is a requirement for "Format Document..." (although I'm not 100% sure this is still the case since they came out with the Black VS Code extension.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, ruff is a linter, not a type checker, so I think we want to keep mypy, at least for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, ruff is a linter, not a type checker, so I think we want to keep mypy, at least for now.

Yes, I was more thinking about mypy being on the slow side to use in a pre-commit workflow, not about eliminating it entirely. I don't want to touch mypy for this PR in any case, one thing at a time is enough for me 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's still useful to have in the dev environment. If you're running VS Code, then black is a requirement for "Format Document..." (although I'm not 100% sure this is still the case since they came out with the Black VS Code extension.)

Interesting. I'm not a VSCode user, but I assumed that would integrate with pre-commit as well. Makes sense!

check-manifest
doctr
flake8
flake8-builtins
flake8-comprehensions
flake8-mutable
build
freezegun
isort
mypy
pre_commit
pylint
pytest
pytest-cov
pytest-flake8
pytest-xdist
pytest-timeout
tomli; python_version<"3.11"
Expand Down