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

Migrate from isort/Black/Flake8 to Ruff for Code Formatting and Linting #5424

Open
wants to merge 26 commits into
base: fix-lrclib-lyrics
Choose a base branch
from

Conversation

snejus
Copy link
Member

@snejus snejus commented Sep 17, 2024

Context

I noticed one of recently merged PRs used unittest assertions that we are migrating from. Thus, I thought it would be a good idea to configure our linting to catch such issues in the future. Since I was here, I replaced our multiple linting and formatting tools with a single tool, Ruff, to simplify and speed up our development workflow.

Summary

This PR migrates the codebase from using isort, Black and Flake8 to Ruff for code formatting and linting. The changes include updates to the GitHub Actions workflow, pre-commit configuration, and documentation to reflect the new tooling.

Changes

  1. GitHub Actions Workflow

    • Updated .github/workflows/lint.yml to use poetry install --only=lint and poe lint --output-format=github.
  2. Pre-commit Configuration

    • Replaced Black and Isort with Ruff in .pre-commit-config.yaml.
  3. Documentation

    • Updated CONTRIBUTING.rst to reflect the use of Ruff for formatting and linting.
    • Modified instructions for running tests and handling external API requests.
  4. Poetry Configuration

    • Removed Black, Isort, Flake8, and related dependencies from poetry.lock and pyproject.toml.
    • Added Ruff as the new linter and formatter in pyproject.toml.
  5. Setup Configuration

    • Removed Flake8 configuration from setup.cfg.
  6. Git blame

    • Introduced .git-blame-ignore-revs file to keep git blame clean from formatting
      changes. Configure your local beets repository to use this file by running:
    $ git config --local blame.ignoreRevsFile .git-blame-ignore-revs

Benefits

  • Performance: Ruff is known for its speed and efficiency, which should improve the developer experience.
  • Consolidation: Using a single tool for both formatting and linting simplifies the development workflow.

How to Test

  1. Linting and Formatting

    • Run poe check-format to check for formatting issues.
    • Run poe format to format the codebase.
    • Run poe lint to check for linting issues.
  2. Pre-commit Hooks

    • Ensure pre-commit hooks are working correctly by running pre-commit run --all-files.
  3. CI Pipeline

    • Verify that the GitHub Actions workflow completes successfully.

Notes

  • Contributions migrating existing tests from unittest to pytest are welcome.
  • External API requests should be mocked using requests_mock and tested weekly in the integration test suite.

References


This PR aims to streamline our development process by adopting Ruff, a modern and efficient tool for Python code formatting and linting.

@snejus snejus self-assigned this Sep 17, 2024
@snejus snejus force-pushed the configure-ruff branch 10 times, most recently from 292996f to 52507cf Compare September 19, 2024 00:00
Adjust the base URL to perform a '/search' instead of attempting to
'/get' specific lyrics where we're unlikely to find lyrics for the
specific combination of album, artist, track names and the duration (see
https://lrclib.net/docs).

Since we receive an array of matching lyrics candidates, rank them by
their duration similarity to the item's duration, and whether they
contain synced lyrics.
Create 'helpers.ConfigMixin' which sets up testing configuration.
This is helpful for tests (e.g. test_lyrics.py) that only need the
configuration and do not require temp dir.

(#5102) Refactor lyrics tests to fix the issue global beets config
issue.

Additionally, add 'integration_test' mark that can be used to mark tests
that should only run once a week.
Improve requests performance with requests.Session which uses connection
pooling for repeated requests to the same host.

Additionally, this centralizes request configuration, making sure that
we use the same timeout and provide beets user agent for all requests.
Due to request error handling this logic will only run for successful
requests, so we can safely assume the html to be a string.
Tidy up 'Google.is_page_candidate' method and remove 'Google.sluggify'
method which was a duplicate of 'slug'.

Since 'GeniusFetchTest' only tested whether the artist name is cleaned
up (the rest of the functionality is patched), remove it and move its
test cases to the 'test_slug' test.
- Fix imports
- Fix pytest issues
- Do not assign lambda as variable
- Use isinstance instead of type to check type
- Rename ambiguously named variables
- Name custom errors with Error suffix
* Replace `noqa` comments in `assert...` method definitions with
  a configuration option to ignore these names.
* Use the `__all__` variable to specify importable items from the
  module, replacing `*` imports and `noqa` comments for unused imports.
* Address issues with poorly named variables and methods by renaming
  them appropriately.
The variable in `test_ordered_enum` was flagged for naming issues, and
I noticed that `OrderedEnum` is essentially `enum.IntEnum`.

I guess `OrderedEnum` exists because it was created before
`enum.IntEnum` was made available in Python 3.4. We do not need it
anymore though, so it's now gone.
This commit introduces a `.git-blame-ignore-revs` file to the
repository. The purpose of this file is to list specific commit hashes
that should be ignored when using the `git blame` command. This is
useful for ignoring commits that involve large-scale formatting changes,
refactoring, or other non-functional changes that would otherwise
clutter the blame history.

I added a couple of previous commit hashes which reformatted the code.

Configure this repository to use this file:

  git config --local blame.ignoreRevsFile .git-blame-ignore-revs
Copy link
Member

@bal-e bal-e left a comment

Choose a reason for hiding this comment

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

For completeness, I've reviewed every file by hand. In almost all cases, I significantly prefer the changes Ruff has introduced. I'm happy with this PR overall -- well done @snejus!


.. code-block:: python
In order to add such test, mark your test with the ``integration_test`` marker
Copy link
Member

Choose a reason for hiding this comment

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

typo: replace with "In order to add such a test"

"Invalid `release_date` returned "
"by {} API: '{}'".format(self.data_source, release_date)
f"Invalid `release_date` returned by {self.data_source} API: "
f"{release_date!r}"
Copy link
Member

Choose a reason for hiding this comment

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

The !r component is new, is it intentional?

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