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

MNT: Switch from flake8 to ruff for linting #342

Merged
merged 3 commits into from
Jan 25, 2024
Merged

Conversation

brendan-ward
Copy link
Member

Resolves #341

Similar to changes in GeoPandas #2921 but without needing to configure additional rules.

Copy link
Member

@martinfleis martinfleis left a comment

Choose a reason for hiding this comment

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

Thanks! We could potentially also consider switching from black to ruff-format. I am using it in some other projects and the transition was flawless.

@brendan-ward
Copy link
Member Author

Switched to ruff-format and removed unnecessary # noqa statements.

@theroggy
Copy link
Member

Maybe add ruff as dependency in environment-dev.yml?

- id: flake8
language: python_venv
- id: ruff
- id: ruff-format
Copy link
Member

Choose a reason for hiding this comment

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

Should we put the formatter first? (eg I can imagine that the linter complains about a too long line, which would already be solved by the formatter afterwards)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure; was just following the example.

I'm assuming we don't want to auto-fix lint errors, which is where putting the linter first is recommended.

@jorisvandenbossche
Copy link
Member

Maybe add ruff as dependency in environment-dev.yml?

pre-commit is included, and then the idea is that you install the pre-commit, which will handle getting ruff (that's also how it went for black and flake8, which aren't included in the yml)

Of course if you don't use pre-commit and want to run ruff manually, you also need it in your env.

@theroggy
Copy link
Member

theroggy commented Jan 24, 2024

Maybe add ruff as dependency in environment-dev.yml?

pre-commit is included, and then the idea is that you install the pre-commit, which will handle getting ruff (that's also how it went for black and flake8, which aren't included in the yml)

Of course if you don't use pre-commit and want to run ruff manually, you also need it in your env.

I personally use visual studio code and then it is convenient that formatter and linter are installed so the formatting is automatically applied and linting errors are shown in the UI. Once you commit pre-commit will check this as well... but this is later in the process.

But, if this is rather typical for my workflow, no problem not to include it... not problem to install it myself...

@brendan-ward
Copy link
Member Author

Given that we had black in the environment-dev.yml (and I removed here), I have no issues with adding ruff as a development dependency. For vscode, I've been using the ruff extension, which I believe ships with its own install of ruff that would be used to highlight things in the vscode UI.

@jorisvandenbossche
Copy link
Member

I don't mind ruff being there in the dev env file (I don't use that anyway, I have a "geo-dev" env that I use for geopandas etc as well), but just a note for those using it: you might have to take care in ensuring the same ruff version is used as in the pre-commit, otherwise you might still get lint failures on CI.

@jorisvandenbossche jorisvandenbossche merged commit db1808d into main Jan 25, 2024
46 checks passed
@jorisvandenbossche jorisvandenbossche deleted the use_ruff branch January 25, 2024 07:53
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.

MNT: Change linting / precommit to use ruff
4 participants