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

Replace black, flake8 and isort with ruff #95

Merged
merged 7 commits into from
Oct 1, 2024
Merged

Replace black, flake8 and isort with ruff #95

merged 7 commits into from
Oct 1, 2024

Conversation

dc2917
Copy link
Contributor

@dc2917 dc2917 commented Oct 1, 2024

This PR removes development dependencies black, flake8 and isort, and replaces them with ruff. The associated pre-commit hooks have also been replaced with ruff's.

Satisfying ruff required adding quite a few docstrings, so it's probably worth checking the docstrings I've added make sense!

Closes #73

pyproject.toml Outdated
@@ -56,6 +54,18 @@ mkdocs-gen-files = "^0.4.0"
mkdocs-literate-nav = "^0.5.0"
mkdocs-section-index = "^0.3.4"

[tool.ruff]
target-version = "py312"
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are meant to support from python 3.9, so target version should be that one. Otherwise, there will be functionality that is not backwards compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, sorry, overlooked that one

@dc2917 dc2917 requested a review from dalonsoa October 1, 2024 13:23
@@ -96,11 +98,11 @@ def read_metadata(


def read_to_array(
filename: Union[Path, str],
filename: Path | str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In theory, this syntax is not valid for Python 3.9. See https://www.slingacademy.com/article/union-type-in-python-the-complete-guide/#:~:text=Introduced%20in%20Python%203.10%20as%20part%20of%20PEP

I think this might be running the ruff pre-commit with the wrong python version. Could you add a default python version in the pre-commit-congig.yml file? See https://pre-commit.com/#top_level-default_language_version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So in readers.py you've got from __future__ import annotations, so the | operator should be valid. Not sure why it's not complaining in writers.py though!

That said, when I run ruff check --target-version py39 it doesn't complain - weird!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot about the __futures__ stuff. Yeah, with that all of that should work. But I agree it should fail if not imported.

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

Let's go ahead with it. The code looks good, so we will see if it fails later on and fix accordingly.

@dc2917 dc2917 merged commit 0ff7bf4 into develop Oct 1, 2024
6 checks passed
@dc2917 dc2917 deleted the use_ruff branch October 1, 2024 14:15
@dalonsoa
Copy link
Collaborator

dalonsoa commented Oct 1, 2024

@all-contributors please add @dc2917 for infra and code

Copy link
Contributor

@dalonsoa

I've put up a pull request to add @dc2917! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updates linters and formatters to use ruff
2 participants