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

Lint with ruff #113

Merged
merged 9 commits into from
Jan 12, 2023
Merged

Lint with ruff #113

merged 9 commits into from
Jan 12, 2023

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented Jan 10, 2023

Add the ruff linter to the development workflow.

Includes basic configuration as well as some minor tweaks/improvements to pass the linter. The configuration is minimal and reflects the original pytest-pep8 configuration.

runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Install Python
Copy link
Member

Choose a reason for hiding this comment

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

In a lot of other workflows, we cache the python environment to speed up action times https://github.com/marketplace/actions/cache. Would that be appropriate to incorporate here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO it's probably not worth it for this case - this should really only install 2 pkgs (updated pip + ruff), neither of which is particularly large thus is unlikely to be a bottleneck.

python -m pip install ruff
python -m pip list
- name: Run ruff
run: ruff --format=github .
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify, this just generates a report and does not actually change any files. The action will fail if the linting standards are not met?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. ruff does have the option to automatically "fix" some subset of errors, though in my experimentation with it some of the autofixes were not correct or otherwise overly aggressive. There are ways to control this in the ruff configs (see e.g. unfixable), but IMO it'd be easiest to start off without it. We can always revisit this once we gain more experience with the linter in place.

@msschwartz21
Copy link
Member

One additional question, can you tell if .pylintrc is being used by anything or if we can delete it?

@rossbar
Copy link
Contributor Author

rossbar commented Jan 12, 2023

One additional question, can you tell if .pylintrc is being used by anything or if we can delete it?

Nice catch, I missed that file. We could remove it as pylint is used explicitly anywhere, and ruff covers all the same bases but is much, much faster.

I'd vote to leave it there for now as it sets up additional linting beyond what ruff is currently configured to do, and we can use it as a guidepost for enabling more features in ruff.

@rossbar rossbar merged commit 49d8ca2 into vanvalenlab:master Jan 12, 2023
@rossbar rossbar deleted the lint-with-ruff branch January 12, 2023 19:00
@msschwartz21 msschwartz21 added the chore Maintenance label Jul 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants