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

#88: Add Ruff! #106

Merged
merged 5 commits into from
Oct 2, 2023
Merged

#88: Add Ruff! #106

merged 5 commits into from
Oct 2, 2023

Conversation

perrygoy
Copy link
Member

Ruff is pretty neat. It's got a lot of different linters all in one spot, and it runs super freakin' quick.

This PR adds ruff with our own stew of selected-and-ignored rules and rulesets.

@perrygoy perrygoy marked this pull request as draft September 23, 2023 03:08
@perrygoy perrygoy changed the title #88: add ruff and pre-commit hook. #88: Add Ruff! Sep 23, 2023
pyproject.toml Outdated
]
include = [
"screenpy",
"tests",
Copy link
Contributor

@bandophahita bandophahita Sep 25, 2023

Choose a reason for hiding this comment

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

For some reason setting this has a negative impact on being able to run ruff against the whole project. We might consider leaving it out. Or just adding "docs" to the exclude.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific? I'm able to run the pre-commit hook without any issues with these settings.

pyproject.toml Outdated
"FBT", # flake8-boolean-trap
"FIX", # flake8-fixme
"FLY", # flynt
"FURB", # refurb
Copy link
Contributor

@bandophahita bandophahita Sep 25, 2023

Choose a reason for hiding this comment

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

I get the following warning with this ruleset.
warning: Selection `FURB` has no effect because the `--preview` flag was not included. Are you getting that too?

My suggestion would be to avoid preview rulesets for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

--preview is for the rules that are still being workshopped. This one used to be fine, but got moved there—i had used it on my other project and it was real nice.

It might be annoying, though, when this is removed and you have to fix the issues in the middle of a different PR. I'll comment it out like you had, in a "we want this someday but not yet" section.

pyproject.toml Outdated
"B", # flake8-bugbear
"BLE", # flake8-blind-except
"C4", # flake8-comprehensions
"D", # pydocstyle
Copy link
Contributor

Choose a reason for hiding this comment

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

We get a lot of errors on the test modules. Personally I don't think we need to run this rule against any of the tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah... the pre-commit hooks only consider the screenpy/** files to be --all-files, this is what was tripping me up.

I'll remove tests from the directories to ruff up.

pyproject.toml Outdated
]
ignore = [
"D107", # missing __init__ docstring, we do that in the class docstring.
"D203", # one blank line before class; we want 2!
Copy link
Contributor

@bandophahita bandophahita Sep 25, 2023

Choose a reason for hiding this comment

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

We want no blank lines before the class, don't we?

class Something:
"""blah blah"""

What you're describing would look like this:

class Something:


"""blah blah"""

And afaik, there isn't a rule to enforce that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, uh, i misunderstood the rule. Ruff said that it was conflicting with another rule that suggested 2 blank lines.

I'll update the comment to reflect the real world.

pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
@bandophahita
Copy link
Contributor

bandophahita commented Sep 25, 2023

This would be my suggested rule adjustments. Some of the super opinionated rules could be left out. There are also a couple of rulesets that will conflict with 3.8 & 3.9.

[tool.ruff]
target-version = "py38"  # minimum supported version
line-length = 88  # same as Black.
select = [
    "A",  # flake8-builtins
    "ANN",  # flake8-annotations
    "B",  # flake8-bugbear
    "C4",  # flake8-comprehensions
    "D",  # pydocstyle
    "E",  # pycodestyle error
    "ERA",  # eradicate
    "F",  # Pyflakes
    "FIX",  # flake8-fixme
    "FLY",  # flynt
    "I",  # isort
    "ICN",  # flake8-import-conventions
    "ISC",  # flake8-implicit-str-concat
    "PGH",  # pygrep-hooks
    "PIE",  # flake8-pie
    "PERF",  # perflint
    "PL",  # pylint
    "PT",  # flake8-pytest-style
    "Q",  # flake8-quotes
    "RET",  # flake8-return
    "RSE",  # flake8-raise
    "RUF",  # ruff specific
    "SIM",  # flake8-simplify
    "T10",  # flake8-debugger
    "T20",  # flake8-print
    "TCH",  # flake8-type-checking
    "TRY",  # tryceratops
    "W",  # pycodestyle warning
    "YTT",  # flake8-2020
    
    
    # would leave off
#    "FURB",  # refurb  # requires use of --preview
#    "ARG",  # flake8-unused-arguments  # more false positives than helpful
#    "BLE",  # flake8-blind-except  # we already use # pylint: disable=broad-except
#    "EM",  # flake8-errmsg  # very opinionated
#    "FA",  # flake8-future-annotations  # conflicts with 3.8 & 3.9
#    "FBT",  # flake8-boolean-trap  # opinionated
#    "UP",  # python upgrade  # conflicts with 3.8
]
ignore = [
    "D107",  # missing __init__ docstring, we do that in the class docstring.
    "D203",  # one blank line before class; we want 2!
    "D212",  # multi line summary first line, we want a one line summary.
    "ANN101",  # missing self annotation, we only annotate self when we return it.
    "ANN401",  # no `**kwargs: Any` typing; unfortunately we don't have a better way.
    
    # should include to avoid issues
    
    # maybe just ignore in the files where needed?
    "A003", # Class attribute shadow builtin   
    "ANN102",  # cls  # mypy doesnt need it. will always be 'Self'
    "D200", # One-line docstring should fit on one line
    "PLR2004", # magic-value-comparison  # super opinionated
    
    # explicit is always better
    "PT003", # [*] `scope='function'` is implied in `@pytest.fixture()`
    "PT011", # `pytest.raises(ValueError)` is too broad -- REALLY?!
]   
# when we get done, we're gonig to want to be able to run `ruff .` against the whole
# project in github actions or tox.  For some reason setting these doesn't allow me to
# run `ruff .`
#include = [
#    "screenpy",
#    "tests",
#]
exclude = [
    "screenpy/__init__.py",
    "screenpy/__version__.py",
    "docs",
]


[tool.ruff.per-file-ignores]
"tests/*" = ["D"]
"setup.py" = ["D"]
"screenpy/actions/debug.py" = ["T100"] # T100 `pdb`, `breakpoint` found/used

@perrygoy perrygoy marked this pull request as ready for review September 26, 2023 03:25
@perrygoy
Copy link
Member Author

@bandophahita ready for review! Thank you for your thoughts—i rearranged the includes/excludes, commented out FURB until it's ready for prime time, and added tests to the mix.

I actually like and agree with the opinions in the rulesets you marked as opinionated, we can talk more about those if you'd like. I've used them in other projects and found the code much easier to read, maintain, and understand with their suggestions.

@perrygoy perrygoy merged commit 032df28 into trunk Oct 2, 2023
19 checks passed
@perrygoy perrygoy deleted the 88/implement-ruff branch October 2, 2023 22:01
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.

2 participants