Replace flake with ruff #154
Conversation
for more information, see https://pre-commit.ci
| - id: flake8 | ||
| args: ["--ignore=E203,F401,W503", --max-line-length=88] | ||
| - id: ruff | ||
| args: [--fix, --exit-non-zero-on-fix] |
There was a problem hiding this comment.
this makes ruff automatically remove unused imports
There was a problem hiding this comment.
see the remove imports here f337f74
| "EigenReporterConfig", | ||
| "RunConfig", | ||
| "OptimConfig", | ||
| ] |
There was a problem hiding this comment.
this makes ruff automatically remove unused imports
but that also means that you need to explicitly export imports
There was a problem hiding this comment.
Oh this is a bit annoying actually...
ChatGPT suggests that you can automate this like so:
# Automatically fill the __all__ field
__all__ = [name for name, value in globals().items() if not name.startswith('__') and callable(value)]
appending this to the end of the __init__ files
There was a problem hiding this comment.
ruff itself (and the user's own linters) wouldn't be able to understand that. That you've reeexported everything inside.
So whoever uses the elk pacakge will still get warnings, and ruff itself will warn us too. (False positive but annoying for everyone)
I've added
ignore-init-module-imports = true
to the pyproject.toml. So what ruff will do is to tell us that we've imported something in __init__.py but didn't add it to__all__. That should fix future mistakes when we forget to declare it.
An example of a message
elk/extraction/__init__.py:1:31: F401 .balanced_sampler.BalancedSampler imported but unused; consider adding to __all__ or using a redundant alias
There was a problem hiding this comment.
the alternative to this would be to ignore all the import warnings in __init__.py files with ruff. I think thats a worse option. because the user's linters/ ide / typecheckers all complain
# Conflicts: # elk/__init__.py # elk/evaluation/evaluate.py # elk/extraction/__init__.py # elk/extraction/prompt_dataset.py # elk/training/__init__.py # elk/utils/__init__.py
for more information, see https://pre-commit.ci
| Dataset, | ||
| Features, | ||
| IterableDataset, | ||
| Sequence, | ||
| ) | ||
| from datasets.distributed import split_dataset_by_node |
There was a problem hiding this comment.
unused imports
Yep, ruff is pretty fast and nice
Still evaluating the docstrings feature
I don't think enforcing rules "D" for docs everywhere is a good idea. There's still quite alot of false positives.
E.g. Ruff will complain about child override methods not having a docstring when the parent has it.
astral-sh/ruff#970 (comment)
Closes #143