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

CHG: switch to ruff format instead of black entirely #3435

Merged
merged 18 commits into from
Jan 17, 2024

Conversation

sbrugman
Copy link
Contributor

Description

Recommend ruff format instead of black
Benefits performance as well as fewer dependencies that can conflict (e.g. click)

Link to ruff's pre-commit version to avoid having to update the docs every time there is a new release, which are frequent.

Development notes

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the RELEASE.md file
  • Added tests to cover my changes
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

🚀 Yes! Approving this with some nitpicks

docs/source/development/linting.md Outdated Show resolved Hide resolved
docs/source/development/linting.md Outdated Show resolved Hide resolved
docs/source/development/linting.md Outdated Show resolved Hide resolved
@astrojuanlu
Copy link
Member

#3395 is getting annoying...

@merelcht
Copy link
Member

I really like this suggestion, but we should then also update the behaviour of our lint tool, because that still uses black. As a user I would be confused that ruff format is the recommendation in the docs, but the tool we provide has a different setup.

@sbrugman sbrugman changed the title DOC: recommend ruff format instead of black CHG: switch to ruff format instead of black entirely Dec 21, 2023
@sbrugman
Copy link
Contributor Author

@merelcht Agreed, better to switch to ruff for both linting and formatting completely.
bandit is now also replaced with ruff

I found some obviated noqa comments and removed them.

It would be good to create a follow-up story that expands the selection of ruff rules, since we only use a subset of the rich library

Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

Thank you so much for switching everything to ruff format @sbrugman ⭐ I left some minor comments, but otherwise this looks good to go! 👍

I also agree on creating a follow up to leverage ruff more where we can.

RELEASE.md Outdated Show resolved Hide resolved
tests/framework/cli/test_starters.py Outdated Show resolved Hide resolved
@merelcht
Copy link
Member

merelcht commented Jan 3, 2024

The tests are currently failing because the pyproject.toml files in kedro-starters need to be updated to contain all ruff setup as well.

Co-authored-by: Merel Theisen <[email protected]>
Signed-off-by: Simon Brugman <[email protected]>
@merelcht
Copy link
Member

The tests are currently failing because the pyproject.toml files in kedro-starters need to be updated to contain all ruff setup as well.

Created a PR for this kedro-org/kedro-starters#207

@merelcht merelcht requested review from noklam and ankatiyar and removed request for yetudada January 16, 2024 11:43
Signed-off-by: Merel Theisen <[email protected]>
Copy link
Contributor

@noklam noklam left a comment

Choose a reason for hiding this comment

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

🙏Appreciate your help!

@SajidAlamQB
Copy link
Contributor

SajidAlamQB commented Jan 16, 2024

Added a change so that unit tests now use the main branch of kedro-starters now its only failing on coverage for thread_runner.py.

@merelcht merelcht enabled auto-merge (squash) January 17, 2024 13:20
@merelcht merelcht merged commit 42b526e into kedro-org:main Jan 17, 2024
35 checks passed
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.

6 participants