astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.38k stars 1.08k forks source link

ruff check src/ --fix fails to discover some nested files but works when ran directly on that file #9504

Open ringohoffman opened 9 months ago

ringohoffman commented 9 months ago
$ ruff --version
ruff 0.1.13
  1. clone nerfstudio.
  2. Use this ruff config:
    
    [tool.ruff]
    line-length = 120
    select = [
    "E",  # pycodestyle errors.
    "F",  # Pyflakes rules.
    "I",  # isort formatting.
    "PLC",  # Pylint convention warnings.
    "PLE",  # Pylint errors.
    "PLR",  # Pylint refactor recommendations.
    "PLW",  # Pylint warnings.
    ]
    ignore = [
    "E501",  # Line too long.
    "F722",  # Forward annotation false positive from jaxtyping. Should be caught by pyright.
    "F821",  # Forward annotation false positive from jaxtyping. Should be caught by pyright.
    "PLR2004",  # Magic value used in comparison.
    "PLR0915",  # Too many statements.
    "PLR0913",  # Too many arguments.
    "PLC0414",  # Import alias does not rename variable. (this is used for exporting names)
    "PLC1901",  # Use falsey strings.
    "PLR5501",  # Use `elif` instead of `else if`.
    "PLR0911",  # Too many return statements.
    "PLR0912",  # Too many branches.
    "PLW0603",  # Globa statement updates are discouraged.
    "PLW2901",  # For loop variable overwritten.
    ]

[tool.ruff.lint.isort] combine-as-imports = true known-first-party = ["nerfstudio"] split-on-trailing-comma = false

3. Apply `ruff` import formatting
```console
$ ruff check nerfstudio/ docs/ tests/ --fix
  1. Use this isort config:
    [tool.isort]
    combine_as_imports = true
    line_length = 120
    known_first_party = ["nerfstudio"]
    profile = "black"
  2. isort discovers a missed fix
    $ isort nerfstudio/ docs/ tests/
    Fixing /Users/ringo/Repositories/nerfstudio/nerfstudio/scripts/downloads/download_data.py
  3. Strangely enough:
    $ ruff check nerfstudio/scripts --fix
    Found 1 error (1 fixed, 0 remaining).

    or

    $ ruff check nerfstudio/scripts/downloads/download_data.py --fix
    Found 1 error (1 fixed, 0 remaining).

And also, ruff succeeds at finding and fixing other scripts in this folder.

$ tree nerfstudio/scripts/
nerfstudio/scripts/
├── __init__.py
├── completions
│   ├── __init__.py
│   ├── install.py
├── downloads
│   ├── __init__.py
│   └── download_data.py  // <- only one that is missed?
├── process_data.py
├── render.py
└── viewer
    ├── __init__.py
    └── run_viewer.py
charliermarsh commented 9 months ago

Thanks, really good issue + instructions, I was able to repro.

charliermarsh commented 9 months ago

I think the issue is that nerfstudio/nerfstudio/scripts/downloads is ignored by the .gitignore, which has this:

downloads/
!nerfstudio/scripts/downloads/

@BurntSushi - is the .gitignore incorrect, or is the ignore crate not able to pick that up?

You can add:

[tool.ruff]
respect-gitignore = false

For now as a workaround. (See: https://docs.astral.sh/ruff/settings/#respect-gitignore.)

BurntSushi commented 9 months ago

Just from a quick look on a checkout of nerfstudio, it looks like the ignore crate is handling the rules correctly and whitelisting nerfstudio/scripts/downloads:

$ rg --files --debug 2> /tmp/log | rg nerfstudio/scripts/downloads
nerfstudio/scripts/downloads/download_data.py
nerfstudio/scripts/downloads/__init__.py
$ rg nerfstudio/scripts/downloads /tmp/log
39:rg: DEBUG|ignore::walk|crates/ignore/src/walk.rs:1802: whitelisting ./nerfstudio/scripts/downloads: Whitelist(IgnoreMatch(Gitignore(Glob { from: Some("./.gitignore"), original: "!nerfstudio/scripts/downloads/", actual: "nerfstudio/scripts/downloads", is_whitelist: true, is_only_dir: true })))
$ fd . | rg nerfstudio/scripts/downloads
nerfstudio/scripts/downloads/
nerfstudio/scripts/downloads/__init__.py
nerfstudio/scripts/downloads/download_data.py

Perhaps there is some interaction between ruff and the ignore crate that is causing the issue here. Or there is some other rule in play. I can dig more into this tomorrow.

charliermarsh commented 9 months ago

I think the issue is that once we exclude a directory, we don't recurse further into it (and so we never test its children). Wouldn't it be really expensive to check all children of all excluded directories?