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

Use ruff instead of flake8 #79

Closed
wants to merge 1 commit into from
Closed
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
4 changes: 2 additions & 2 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ concurrency:
jobs:
test:
name: test ${{ matrix.tox_env }}
runs-on: ubuntu-22.04
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
Expand Down Expand Up @@ -42,7 +42,7 @@ jobs:

check:
name: check ${{ matrix.tox_env }}
runs-on: ubuntu-22.04
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
Expand Down
5 changes: 2 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,13 @@ coverage: ## Run the test suite, with Python code coverage

.PHONY: format
format: ## Autoformat all files
$(PYTHON) -m isort $(python_files)
$(PYTHON) -m black $(python_files)
$(PYTHON) -m ruff --fix $(python_files)

.PHONY: lint
lint: ## Lint all files
$(PYTHON) -m isort --check $(python_files)
$(PYTHON) -m flake8 $(python_files)
$(PYTHON) -m black --check --diff $(python_files)
$(PYTHON) -m ruff check $(python_files)
$(PYTHON) -m mypy src/pytest_memray --ignore-missing-imports

.PHONY: docs
Expand Down
15 changes: 9 additions & 6 deletions docs/conf.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
"""Sphinx configuration file for pytest-memray documentation."""
"""Sphinx configuration file for pytest-memray documentation.""" # noqa: INP001
Copy link
Contributor

Choose a reason for hiding this comment

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

Mm. It doesn't have any provision for handling scripts that aren't part of packages? That's annoying...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All python files should be formatted, I don't see why this should be different.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you misunderstood me. I'm not arguing that docs/conf.py shouldn't be formatted, I'm complaining that ruff thinks that docs is a Python package, and wants is to create a docs/__init__.py - it doesn't understand that docs/conf.py is a standalone script that's not part of any package.

from __future__ import annotations

import sys
from pathlib import Path
from subprocess import check_output
from typing import TYPE_CHECKING

from sphinx.application import Sphinx
from sphinxcontrib.programoutput import Command

if TYPE_CHECKING:
from sphinx.application import Sphinx
Comment on lines +11 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be moved below an if TYPE_CHECKING:?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


extensions = [
"sphinx.ext.extlinks",
"sphinx.ext.githubpages",
Expand All @@ -33,11 +36,11 @@
prev = Command.get_output
here = Path(__file__).parent
linkcheck_allowed_redirects = {
"https://github.com/bloomberg/pytest-memray/issues/.*": "https://github.com/bloomberg/pytest-memray/pull/.*"
"https://github.com/bloomberg/pytest-memray/issues/.*": "https://github.com/bloomberg/pytest-memray/pull/.*",
}


def _get_output(self):
def _get_output(self: Command) -> tuple[int, str]:
code, out = prev(self)
out = out.replace(str(Path(sys.executable).parents[1]), "/v")
out = out.replace(str(here), "/w")
Expand All @@ -47,11 +50,11 @@ def _get_output(self):
Command.get_output = _get_output


def setup(app: Sphinx) -> None:
def setup(app: Sphinx) -> None: # noqa: ARG001
here = Path(__file__).parent
root, exe = here.parent, Path(sys.executable)
towncrier = exe.with_name(f"towncrier{exe.suffix}")
cmd = [str(towncrier), "build", "--draft", "--version", "NEXT"]
new = check_output(cmd, cwd=root, text=True)
new = check_output(cmd, cwd=root, text=True) # noqa: S603
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably ignore this rule globally. I have no idea what it's complaining about.

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 locally is better to be explicit that this instance is allowed but not in general. https://beta.ruff.rs/docs/rules/#flake8-bandit-s shows the rules with explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I read the rule explanation. It seems to complain about every call to subprocess, as far as I can see. I tested running it with cmd set to a list of hardcoded strings, and it still raised an S603 error. That doesn't seem useful to me - have I missed something?

to = root / "docs" / "_draft.rst"
to.write_text("" if "No significant changes" in new else new)
10 changes: 5 additions & 5 deletions docs/demo/test_ok.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
from __future__ import annotations
from __future__ import annotations # noqa: INP001
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is in docs/, it's intended for end-user consumption, right? Not just for maintainers? If so, I don't think we should add annotations here to appease our linter. I think we should tell the linter to ignore this file instead.

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 don't agree. Why should not we lint documentation files same as the rest?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because it makes the code harder to read by adding annotations that most of our users won't recognize or care about, which makes it function worse as documentation


import pytest


def test_track():
def test_track() -> None:
from heapq import heappush

h = []
for value in range(1):
heappush(h, value)
assert [1] * 5_000
assert [1] * 5_000 # noqa: S101


@pytest.mark.limit_memory("100 KB")
def test_memory_exceed():
def test_memory_exceed() -> None:
found = [[i] * 1_000 for i in range(15)]
assert found
assert found # noqa: S101
105 changes: 62 additions & 43 deletions pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,61 +1,65 @@
[build-system]
build-backend = "hatchling.build"
requires = ["hatchling>=1.12.2", "hatch-vcs>=0.3"]
requires = [
"hatch-vcs>=0.3",
"hatchling>=1.18",
]

[project]
name = "pytest-memray"
description = "A simple plugin to use with pytest"
readme.file = "README.md"
readme.content-type = "text/markdown"
readme.file = "README.md"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this line moved? Did a tool do this automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

license = "apache-2.0"
urls."Bug Tracker" = "https://github.com/bloomberg/pytest-memray/issues"
urls.Documentation = "https://pytest-memray.readthedocs.io"
urls."Source Code" = "https://github.com/bloomberg/pytest-memray"
authors = [
maintainers = [
{ name = "Pablo Galindo Salgado", email = "[email protected]" },
]
maintainers = [
authors = [
{ name = "Pablo Galindo Salgado", email = "[email protected]" },
]
requires-python = ">=3.8"
dependencies = [
"pytest>=7.2",
"memray>=1.5",
]
optional-dependencies.docs = [
"furo>=2022.12.7",
"sphinx>=6.1.3",
"sphinx-argparse>=0.4",
"sphinx-inline-tabs>=2022.1.2b11",
"sphinxcontrib-programoutput>=0.17",
"towncrier>=22.12",
]
optional-dependencies.lint = [
"black==22.12",
"flake8==6",
"isort==5.11.4",
"mypy==0.991",
]
optional-dependencies.test = [
"covdefaults>=2.2.2",
"pytest>=7.2",
"coverage>=7.0.5",
"flaky>=3.7",
"pytest-xdist>=3.1",
]
dynamic = ["version"]
classifiers = [
"Intended Audience :: Developers",
"License :: OSI Approved :: Apache Software License",
"Operating System :: POSIX :: Linux",
"Programming Language :: Python :: 3 :: Only",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"Programming Language :: Python :: 3.11",
"Programming Language :: Python :: Implementation :: CPython",
"Topic :: Software Development :: Debuggers",
]

dynamic = [
"version",
]
dependencies = [
"memray>=1.8",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we changing from depending on memray >=1.5 to >=1.8? Is this something that should have already been applied in the past and was missed? If so, can you at least make it a separate commit so we can find it when searching through the git log?

Copy link
Contributor Author

@gaborbernat gaborbernat Jun 17, 2023

Choose a reason for hiding this comment

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

Because the CI is only testing the latest version of memray. Anything else is a guess at best, and a straight lie otherwise. We might as well be explicit about what version we support (aka test with). Explicit better than implicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, fine. This should be done in a separate commit, so that your reviewer doesn't need to guess what changes were made automatically by a tool and what changes were made manually by you.

"pytest>=7.3.2",
godlygeek marked this conversation as resolved.
Show resolved Hide resolved
]
optional-dependencies.docs = [
"furo>=2023.5.20",
"sphinx>=7.0.1",
"sphinx-argparse>=0.4",
"sphinx-inline-tabs>=2023.4.21",
"sphinxcontrib-programoutput>=0.17",
"towncrier>=23.6",
]
optional-dependencies.lint = [
"black==23.3",
"mypy==1.3",
"ruff==0.0.272",
]
optional-dependencies.test = [
"covdefaults>=2.3",
"coverage>=7.2.7",
"flaky>=3.7",
"pytest>=7.3.2",
"pytest-xdist>=3.3.1",
]
urls."Bug Tracker" = "https://github.com/bloomberg/pytest-memray/issues"
urls.Documentation = "https://pytest-memray.readthedocs.io"
urls."Source Code" = "https://github.com/bloomberg/pytest-memray"
[project.entry-points.pytest11]
memray = "pytest_memray.plugin"

Expand Down Expand Up @@ -91,15 +95,6 @@ python_version = "3.8"
show_error_codes = true
strict = true

[tool.isort]
force_single_line = true
multi_line_output = 3
include_trailing_comma = true
force_grid_wrap = 0
use_parentheses = true
line_length = 88
known_first_party = ["pytest_memray"]

[tool.towncrier]
name = "pytest-memray"
filename = "docs/news.rst"
Expand All @@ -114,3 +109,27 @@ type = [
{ name = "Improved Documentation", directory = "doc", showcontent = true },
{ name = "Miscellaneous", directory = "misc", showcontent = true },
]

[tool.ruff]
select = ["ALL"]
line-length = 88
target-version = "py38"
isort = {known-first-party = ["pytest_memray"], required-imports = ["from __future__ import annotations"]}
ignore = [
"ANN401", # dynamically typed expression
"D", # documentation not always applied
"ANN101", # self type annotation
"D203", # `one-blank-line-before-class` (D203) and `no-blank-line-before-class` (D211) are incompatible
"D212", # `multi-line-summary-first-line` (D212) and `multi-line-summary-second-line` (D213) are incompatible
"S104", # Possible binding to all interface
]
[tool.ruff.per-file-ignores]
"tests/**/*.py" = [
"S101", # asserts allowed in tests...
"FBT", # don"t care about booleans as positional arguments in tests
"INP001", # no implicit namespace
"D", # don"t care about documentation in tests
"S603", # `subprocess` call: check for execution of untrusted input
"PLR2004", # Magic value used in comparison, consider replacing with a constant variable
]
"src/pytest_memray/_version.py" = ["I002"] # Missing required import: `from __future__ import annotations
2 changes: 1 addition & 1 deletion src/pytest_memray/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from __future__ import annotations

from ._version import __version__ as __version__
from ._version import __version__

__all__ = [
"__version__",
Expand Down
23 changes: 14 additions & 9 deletions src/pytest_memray/marks.py
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
from __future__ import annotations

from dataclasses import dataclass
from typing import Tuple
from typing import cast
from typing import TYPE_CHECKING, Tuple, cast

from memray import AllocationRecord
from pytest import Config
from .utils import parse_memory_string, sizeof_fmt, value_or_ini

from .utils import parse_memory_string
from .utils import sizeof_fmt
from .utils import value_or_ini
if TYPE_CHECKING:
import pytest
from memray import AllocationRecord

PytestSection = Tuple[str, str]

Expand Down Expand Up @@ -62,7 +60,10 @@ def long_repr(self) -> str:


def limit_memory(
limit: str, *, _allocations: list[AllocationRecord], _config: Config
limit: str,
*,
_allocations: list[AllocationRecord],
_config: pytest.Config,
) -> _MemoryInfo | None:
"""Limit memory used by the test."""
max_memory = parse_memory_string(limit)
Expand All @@ -72,7 +73,11 @@ def limit_memory(
num_stacks: int = cast(int, value_or_ini(_config, "stacks"))
native_stacks: bool = cast(bool, value_or_ini(_config, "native"))
return _MemoryInfo(
max_memory, total_allocated_memory, _allocations, num_stacks, native_stacks
max_memory,
total_allocated_memory,
_allocations,
num_stacks,
native_stacks,
)


Expand Down
Loading