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

MAINT: Test using ruff with minimal changes to defaults #127

Closed
wants to merge 2 commits into from

Conversation

MridulS
Copy link
Member

@MridulS MridulS commented Apr 16, 2024

This adds an example ruff config which uses all the defaults with minimal config. It makes very little changes.

@jl-wynen
Copy link
Member

Note that this uses much fewer rules than scipp/scipp#3424 In particular, it does not use rules based on isort, bugbear, and bandit. So we still need to run those linters.

@MridulS
Copy link
Member Author

MridulS commented Apr 16, 2024

does not use rules based on isort, bugbear, and bandit. So we still need to run those linters.

urghh, for some reason I thought the default was running all the rules (it's not). Need to select ALL.

docs/conf.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
"PT", "PYI", "RUF", "UP", "W"]
ignore = [
"E111", "E114", "E117", "D206", "D300", # conflict with ruff format
"UP007"] # FIXME: PEP 604 breaks esssans test suite
Copy link
Member Author

Choose a reason for hiding this comment

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

Alright the problem is that this rule only works for python 3.10+ which essans satisfies but in the test suite it uses a released version of sciline which is on py3.9.

So I end up with errors like E TypeError: 'type' object is not subscriptable locally.

Copy link
Member

Choose a reason for hiding this comment

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

Why would this matter? We don't lint our dependencies, are we?

@@ -248,7 +247,7 @@
")\n",
"parts = (CleanSummedQ[SampleRun, Numerator], CleanSummedQ[SampleRun, Denominator])\n",
"iofqs = (IofQ[SampleRun], IofQ[BackgroundRun], BackgroundSubtractedIofQ)\n",
"keys = monitors + (MaskedData[SampleRun],) + parts + iofqs\n",
"keys = (*monitors, MaskedData[SampleRun], *parts, *iofqs)\n",
Copy link
Member

Choose a reason for hiding this comment

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

Why is this preferred...?

Copy link
Member

Choose a reason for hiding this comment

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

It's more efficient. Plus it avoids the awkward (x,) syntax.

@@ -20,14 +19,11 @@
"""


def write_dependencies(dependency_name: str, dependencies: List[str]) -> None:
def write_dependencies(dependency_name: str, dependencies: list[str]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this preferred...? Or is it only for consistent type-hinting...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Using generics is deprecated behaviour https://docs.astral.sh/ruff/rules/non-pep585-annotation/

@MridulS
Copy link
Member Author

MridulS commented Apr 25, 2024

I'll close this one, once scipp/scipp#3424 is merged in we should create one in the template and let the github action do the work.

@MridulS MridulS closed this Apr 25, 2024
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.

3 participants