search

astral-sh / ruff-pre-commit

A pre-commit hook for Ruff.
Apache License 2.0
919 stars 41 forks source link

Add `jupyter` and `toml` file types #44

Closed hamdanal closed 1 year ago

hamdanal commented 1 year ago

Fixes #43

Summary

Add jupyter to the file types that ruff pre-commit hook runs on

Test Plan

This is what pre-commit uses to identify ".ipynb" files. See https://github.com/pre-commit/identify/blob/dc75a76b7fd7093a7a1d4159d6741884343af694/identify/extensions.py#L100 Also black does this https://github.com/psf/black/blob/main/.pre-commit-hooks.yaml

charliermarsh commented 1 year ago

Thanks! Do you mind adding toml too?

charliermarsh commented 1 year ago

Thanks!

hamdanal commented 1 year ago

Hi @charliermarsh, does ruff lint toml files? Or did you ask for toml to be added so that ruff has access to the configuration in pyproject.toml?

I tried this with one of my projects

- repo: https://github.com/astral-sh/ruff-pre-commit
  # Ruff version.
  rev: v0.0.277
  hooks:
    - id: ruff
      types_or: [python, pyi, jupyter, toml]
      args: [--fix, --exit-non-zero-on-fix]

And ruff started failing on the poetry.lock file (which is recognized by pre-commit as a toml file). It thinks this file is a python file and fails with a syntax error. I am not sure toml should be added here.

Thank you for ruff by the way, it is an amazing tool.

charliermarsh commented 1 year ago

Yeah, Ruff lints pyproject.toml files (but no other TOML files). Hmm, we may need to revert that for now since we won't have a reliable way to detect TOML files if they're not using the .toml suffix.

hamdanal commented 1 year ago

I'll send a PR to revert it and see how can we add pyproject.toml only.

charliermarsh commented 1 year ago

Can you refresh my memory -- if we revert both of these, what happens if someone adds this to their configuration? Will Ruff still end up linting Jupyter notebooks?

- repo: https://github.com/astral-sh/ruff-pre-commit
  # Ruff version.
  rev: v0.0.277
  hooks:
    - id: ruff
      types_or: [python, pyi, jupyter]
      args: [--fix, --exit-non-zero-on-fix]
charliermarsh commented 1 year ago

We may actually want to revert Jupyter too for now -- it should be considered opt-in, not on-by-default.

hamdanal commented 1 year ago

Can you refresh my memory -- if we revert both of these, what happens if someone adds this to their configuration? Will Ruff still end up linting Jupyter notebooks?

- repo: https://github.com/astral-sh/ruff-pre-commit
  # Ruff version.
  rev: v0.0.277
  hooks:
    - id: ruff
      types_or: [python, pyi, jupyter]
      args: [--fix, --exit-non-zero-on-fix]

Yes, you can override types_or locally in the user's repository

hamdanal commented 1 year ago

We may actually want to revert Jupyter too for now -- it should be considered opt-in, not on-by-default.

What about another hook like what black does. This way jupyter stays opt-in but also users don't have to learn about low level pre-commit configuration to make it work with notebooks.

charliermarsh commented 1 year ago

I think my preference for now would be to revert, and add an example to the docs that users can copy-paste to add Jupyter support.

hamdanal commented 1 year ago

I think my preference for now would be to revert, and add an example to the docs that users can copy-paste to add Jupyter support.

Done in #45

charliermarsh commented 1 year ago

Thank you, sorry for the churn!