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

(🎁) add ruff config #469

Merged
merged 1 commit into from
Oct 15, 2023
Merged

(🎁) add ruff config #469

merged 1 commit into from
Oct 15, 2023

Conversation

KotlinIsland
Copy link
Contributor

@KotlinIsland KotlinIsland commented Sep 29, 2023

Ruff covers flake8's ruleset, but also the ruleset of 20 or so other linters, plus it's much much faster, also it can automatically fix a lot of issues. We can remove a bunch of these configs if you think it's too many rules to enforce. We can just stick to the flake8 ones maybe.

https://github.com/astral-sh/ruff
https://docs.astral.sh/ruff/rules

@KotlinIsland KotlinIsland force-pushed the fix-ruff branch 6 times, most recently from 963cb53 to c2baf04 Compare September 29, 2023 16:47
@KotlinIsland
Copy link
Contributor Author

@ymyzk Is there any reason to not merge this? not having a lint CI run on pull requests is a little painful

@ymyzk
Copy link
Owner

ymyzk commented Oct 12, 2023

Ah, sorry. I left code review comments, but forgot to click the submit button. Let me do so now.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -7,3 +8,11 @@ module = [
'google.*',
]
ignore_missing_imports = true

[tool.ruff]
select = ["A", "B", "C", "D", "E", "F", "G", "I", "N", "Q", "S", "T", "W", "ARG", "BLE", "COM", "DJ", "DTZ", "EM", "ERA", "EXE", "FURB", "ICN", "INP", "ISC", "NPY", "PD", "PIE", "PL", "PT", "PTH", "PYI", "RET", "RSE", "RUF", "SIM", "SLF", "TCH", "TID", "TRY", "UP", "YTT"]
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer not to maintain a long list of configuration for this project. I've been using flake8 as it's simple and sufficient for this project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ruff covers flake8's ruleset, but also the ruleset of 20 or so other linters, plus it's much much faster, also it can automatically fix a lot of issues. We can remove a bunch of these configs if you think it's too many rules to enforce. We can just stick to the flake8 ones maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I think that isort and pyupgrade are really good ones to keep.

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for elaborating the context. I understand you prefer to use ruff with many rules and I understand people have different preferences on CI tooling.

However, I don't see a need to use so many lint tools for this project considering that the code base is relatively small and simple. Also, we don't have a need to use a faster lint tool. IIUC, it's difficult for a human to notice difference in the time to execute flake8 vs ruff using this project's code base.

My preference for this project is using a minimal set of tool with a minimal set of configurations. I feel the direction of this PR is not much aligned with the direction I want to go...

app/setup.cfg Outdated Show resolved Hide resolved
@ymyzk ymyzk merged commit cf947bb into ymyzk:master Oct 15, 2023
@KotlinIsland KotlinIsland deleted the fix-ruff branch October 15, 2023 05:37
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