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

feat(filter): --cachedir-ignore option #949

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

SIGSTACKFAULT
Copy link

@SIGSTACKFAULT SIGSTACKFAULT commented Apr 24, 2024

Description

Fixes #948.

adds the --cachedir-ignore option which causes eza to ignore directors with a CACHEDIR.TAG and the correct magic number. see https://bford.info/cachedir/

How Has This Been Tested?

nix flake check passes, but I haven't written formal tests. that's why it's a WIP.

TODO

  • Add tests
  • Add completions for bash, zsh, fish, nushell
  • Add documentation to the man page
  • Add your option to the help flag
  • Add your option to the README.md

@SIGSTACKFAULT SIGSTACKFAULT marked this pull request as draft April 24, 2024 21:42
@SIGSTACKFAULT
Copy link
Author

oop i thought adding [WIP] made it a draft automagically

@SIGSTACKFAULT
Copy link
Author

I added a directory with a CACHEDIR.TAG to itest; should I update all the other tests or put the new directory somewhere else?

@SIGSTACKFAULT
Copy link
Author

SIGSTACKFAULT commented Apr 25, 2024

might change it from --cachedir-ignore to --ignore-cachedir because i was copying --git-ignore but it makes more sense as --ignore-cachedir

@SIGSTACKFAULT SIGSTACKFAULT changed the title [WIP] feat(filter): --cachedir-ignore option feat(filter): --cachedir-ignore option Apr 26, 2024
@SIGSTACKFAULT SIGSTACKFAULT marked this pull request as ready for review April 26, 2024 01:22
@MartinFillon
Copy link
Contributor

Hey, first thing, thanks for the pr, I am going to take a deeper look at it wen I have time. Please make sure to write tests cases and rerun the necessary tools.

@PThorpe92
Copy link
Member

Awesome, thanks for the PR!

I like the idea for sure, I'll also check it out later on

Copy link
Contributor

@MartinFillon MartinFillon left a comment

Choose a reason for hiding this comment

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

As this is a feature that reads data could you make it a feature in cargo toml, make it a default, just to be sure it can be disabled by user not wanting any of that

@SIGSTACKFAULT
Copy link
Author

Hey, first thing, thanks for the pr, I am going to take a deeper look at it wen I have time. Please make sure to write tests cases and rerun the necessary tools.

That was one of my questions -- to add a test case i would have to add stuff presumably to the itest directory, but doing that would break all the existing tests. should I update all the existing tests? create a new directory? some third thing?

@SIGSTACKFAULT
Copy link
Author

SIGSTACKFAULT commented Apr 27, 2024

As this is a feature that reads data could you make it a feature in cargo toml, make it a default, just to be sure it can be disabled by user not wanting any of that

as in, a feature that deletes the flag entirely, makes the flag a no-op, makes it not check the magic number, or what? it already only bothers to check for CACHEDIR.TAG if the flag is passed.

@MartinFillon
Copy link
Contributor

as in, a feature that deletes the flag entirely, makes the flag a no-op, makes it not check the magic number, or what

yes

@SIGSTACKFAULT
Copy link
Author

which one do you mean by "yes"? i don't understand

@MartinFillon
Copy link
Contributor

All of them, as I already had previous users reaching out on adding features needing dependencies or having small systems, I think that having the option to disable file reading is not a bad idea, if you have thought lmk.

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

I'd also like to add that before I'd consider this ready to merge, the 16 commits should be squashed into more logical units.

@PThorpe92
Copy link
Member

PThorpe92 commented Apr 30, 2024

Hey finally had a chance to look this over. This looks good man. 👍 Just the one minor fn param.

Seeing as this needs the flag to be enabled, I dont see a reason why it needs to be behind a feature flag.

I do agree with cafk, if we could rebase some of those commits into a couple/few logical conventional commits. This is good to go for me 👍

src/fs/filter.rs Outdated Show resolved Hide resolved
PThorpe92
PThorpe92 previously approved these changes Apr 30, 2024
@MartinFillon
Copy link
Contributor

Seeing as this needs the flag to be enabled, I dont see a reason why it needs to be behind a feature flag.

I will take a look at the code to be sure but I trust you as you read the code, didn't had time for the moment tho.

@SIGSTACKFAULT
Copy link
Author

is that squashed enough or do you want more?

@MartinFillon
Copy link
Contributor

Looks good enough

@PThorpe92 PThorpe92 enabled auto-merge (rebase) May 6, 2024 14:56
Copy link
Contributor

@MartinFillon MartinFillon left a comment

Choose a reason for hiding this comment

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

Could you please add at least testing files to the test generator, and if possible please run the powertest binary at the root at the repo, for an example of the way to follow please see #959

@SIGSTACKFAULT
Copy link
Author

SIGSTACKFAULT commented May 7, 2024

Yeah, i know it needs tests. i've been nerd-sniped by bevy, sorry >.<

@SIGSTACKFAULT SIGSTACKFAULT marked this pull request as draft May 7, 2024 17:45
auto-merge was automatically disabled May 7, 2024 17:45

Pull request was converted to draft

@SIGSTACKFAULT
Copy link
Author

just itest fails on main so i think i'm waiting for #959

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

feat: option to ignore directories with CACHEDIR.TAG
4 participants