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

Line length in PR is currently not checked? #7195

Closed
lagru opened this issue Oct 10, 2023 · 2 comments · Fixed by #7197
Closed

Line length in PR is currently not checked? #7195

lagru opened this issue Oct 10, 2023 · 2 comments · Fixed by #7197
Labels
🤖 type: Infrastructure CI, packaging, tools and automation

Comments

@lagru
Copy link
Member

lagru commented Oct 10, 2023

Our ruff configuration specifies

line-length = 88

but then ignores line-length explicitly in

scikit-image/pyproject.toml

Lines 164 to 165 in c2e2c22

ignore = [
'E501',

Since PR linting with pep8speaks was recently disabled in favor of pre-commit.ci, line length is currently not checked at all in PRs. Do we care?

This seems like an oversight to me and I would have simply made a PR to remove the ignored rule. However, ruff runs on the complete file and not on the diff. This is a problem because I don't want to force contributors to reformat code they haven't touched.

There might be ways to achieve this feature (see astral-sh/ruff#4049 (comment) or https://github.com/dorschw/riff) but they don't look ideal.

I am not sure if I dare suggest this, but we could also re-enable pep8speaks as it supports this use case with its default diff_only: True option.

... or we revive a slightly Sisyphean discussion around introducing a code formatter like black. 😅 This would be my long-term preference.

@lagru lagru added the 🤖 type: Infrastructure CI, packaging, tools and automation label Oct 10, 2023
@lagru
Copy link
Member Author

lagru commented Oct 10, 2023

Noticed this in #7192.

@mkcor
Copy link
Member

mkcor commented Oct 10, 2023

This is a problem because I don't want to force contributors to reformat code they haven't touched.

Absolutely.

I am not sure if I dare suggest this, but we could also re-enable pep8speaks as it supports this use case with its default diff_only: True option.

I'm not opposed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 type: Infrastructure CI, packaging, tools and automation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants