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

Add options to exclude files #238

Merged
merged 11 commits into from
Dec 20, 2024
Merged

Add options to exclude files #238

merged 11 commits into from
Dec 20, 2024

Conversation

LiamPattinson
Copy link
Collaborator

Fixes #233

Opened as a draft as I've yet to add tests.

Doesn't add all features suggested in #233:

@ZedThree
Copy link
Member

I think --include is essentially the positional arguments, basically files?

--force-exclude probably good to get in too, it sounds like it's useful for things like precommit

@@ -69,7 +69,7 @@ unknown-key = 1
|
2 | unknown-key = 1
| ^^^^^^^^^^^
unknown field `unknown-key`, expected one of `files`, `ignore`, `select`, `extend-select`, `per-file-ignores`, `extend-per-file-ignores`, `line-length`, `file-extensions`, `fix`, `no-fix`, `unsafe-fixes`, `no-unsafe-fixes`, `show-fixes`, `no-show-fixes`, `fix-only`, `no-fix-only`, `output-format`, `preview`, `no-preview`, `progress-bar`
unknown field `unknown-key`, expected one of `files`, `fix`, `no-fix`, `unsafe-fixes`, `no-unsafe-fixes`, `show-fixes`, `no-show-fixes`, `fix-only`, `no-fix-only`, `output-format`, `preview`, `no-preview`, `progress-bar`, `ignore`, `select`, `extend-select`, `per-file-ignores`, `extend-per-file-ignores`, `file-extensions`, `exclude`, `extend-exclude`, `line-length`
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is perhaps getting a little long. Looks like ruff suppresses this somehow. Once we start adding individual rule settings, this might just not be helpful.

@ZedThree
Copy link
Member

Should we add some default exclude values? Ruff has:

pub(crate) static EXCLUDE: &[FilePattern] = &[
    FilePattern::Builtin(".bzr"),
    FilePattern::Builtin(".direnv"),
    FilePattern::Builtin(".eggs"),
    FilePattern::Builtin(".git"),
    FilePattern::Builtin(".git-rewrite"),
    FilePattern::Builtin(".hg"),
    FilePattern::Builtin(".ipynb_checkpoints"),
    FilePattern::Builtin(".mypy_cache"),
    FilePattern::Builtin(".nox"),
    FilePattern::Builtin(".pants.d"),
    FilePattern::Builtin(".pyenv"),
    FilePattern::Builtin(".pytest_cache"),
    FilePattern::Builtin(".pytype"),
    FilePattern::Builtin(".ruff_cache"),
    FilePattern::Builtin(".svn"),
    FilePattern::Builtin(".tox"),
    FilePattern::Builtin(".venv"),
    FilePattern::Builtin(".vscode"),
    FilePattern::Builtin("__pypackages__"),
    FilePattern::Builtin("_build"),
    FilePattern::Builtin("buck-out"),
    FilePattern::Builtin("dist"),
    FilePattern::Builtin("node_modules"),
    FilePattern::Builtin("site-packages"),
    FilePattern::Builtin("venv"),
];

We probably want at least the version control ones, maybe some of the others too.

@LiamPattinson
Copy link
Collaborator Author

I think --include is essentially the positional arguments, basically files?

I think it's just a way to put an inclusive filter on the file choices. If you set it to some subset of a project in ruff.toml (src/myproject/some_dir/*.py and then try linting some other non-overlapping subset (src/myproject/some_other_dir/) , it responds by saying no Python files were found. I'm not sure why it's an option in the config file but not on the command line though.

Should we add some default exclude values?

Yes, it's probably worth excluding things like .git and .vscode by default. Perhaps also build and _build, as I think that would work for 90% of fpm and Cmake users?

fortitude/src/cli.rs Outdated Show resolved Hide resolved
@LiamPattinson LiamPattinson marked this pull request as ready for review December 17, 2024 17:02
@LiamPattinson
Copy link
Collaborator Author

I've added a list of default exclude directories and tweaked the logic in get_files a bit to make sure the file patterns work as expected. I also got a few tests in, but fortitude/tests/check.rs is beginning to get a bit unwieldy.

I think that respecting .gitignore will require swapping out walkdir for the ignore crate and rewriting the get_files function accordingly. I think I'd prefer to leave that for its own PR.

README.md Outdated
@@ -152,6 +152,36 @@ preview = true

Run `fortitude explain` to see which rules are in preview mode.

### Excluding Files
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead this should be in docs/index.md?

Actually, looking at ruff, they have some tools for extracting portions of the README.md into the site docs. We should look at that at some point

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, I've moved that section to the docs along with some other things from the README.

README.md Outdated
Comment on lines 160 to 162
Fortitude will automatically ignore files in some directories (`build/`, `.git/`,
`.venv/`, etc.), and this behaviour can be extended using the `--exclude` option. For
example, to ignore all files in the directory `benchmarks/`:
Copy link
Member

Choose a reason for hiding this comment

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

Oh, does --exclude not override the builtins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From playing around with Ruff, apparently not, as it behaves as if the built-ins are a mandatory set of --extend-exclude instead of --exclude. If you want it to lint everything in your venv, you have to manually pass it venv/lib/Python3.10/site-packages, and setting your own exclude doesn't change this behaviour.

I haven't played around with include in a config file, so I'm not sure how that might affect it.

let tempdir = TempDir::new()?;
apply_common_filters!();
// Expect:
// - Overwrite 'foo.f90' in config file, see 'base.f90' and 'foo.f90' but not 'bar.f90'
Copy link
Member

Choose a reason for hiding this comment

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

Lowest possible priority, but you meant this?

Suggested change
// - Overwrite 'foo.f90' in config file, see 'base.f90' and 'foo.f90' but not 'bar.f90'
// - Override 'foo.f90' in config file, see 'base.f90' and 'foo.f90' but not 'bar.f90'

@ZedThree ZedThree merged commit 79559f9 into main Dec 20, 2024
25 checks passed
@ZedThree ZedThree deleted the feature/exclude_files branch December 20, 2024 09:48
@ZedThree ZedThree added the configuration Related to settings and configuration label Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Related to settings and configuration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude files, directories, and patterns
2 participants