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: --ignore-submodule-contents #945

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

llllvvuu
Copy link

@llllvvuu llllvvuu commented Apr 23, 2024

Problem

Resolves #420. Submodules can also be a perf issue since the submodule contents are not affected by the parent .gitignore.

Ref: ogham/exa#1089 ogham/exa#1155

Solution

Add --ignore-submodule-contents. I don't call it --ignore-submodules, since it doesn't solve ogham/exa#1155. Actually that issue is asking for a different feature which could be called --ignore-submodule-status.

Testing

Tested in combination with other flags as well as through symlinks, both manually and in tests/gen. Updated powertest.yaml. Also tested manually through ...

@llllvvuu
Copy link
Author

llllvvuu commented Apr 23, 2024

I don't know how to use Nix, so it's taking some time for me to figure out how to re-generate the stdout/stderr. Once I've sorted that out, I'll move this out of draft. Done

@llllvvuu llllvvuu force-pushed the llllvvuu/hide_submodule_contents branch 5 times, most recently from bc66a6e to 2527365 Compare April 23, 2024 07:27
@llllvvuu llllvvuu marked this pull request as ready for review April 23, 2024 07:33
TODO: handle with --tree
TODO: lazy-load the submodule list
TODO: update docs/help
TODO: doesn't work when relative path passed on command line
TODO: lazy-load the submodule list
TODO: update docs/help
TODO: doesn't work when relative path passed on command line
TODO: update docs/help
TODO: tests
TODO: update docs/help
TODO: tests
TODO: update docs/help
TODO: tests
@llllvvuu llllvvuu force-pushed the llllvvuu/hide_submodule_contents branch from 2527365 to 1cc6ca3 Compare April 23, 2024 07:36
Copy link
Member

@PThorpe92 PThorpe92 left a comment

Choose a reason for hiding this comment

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

Tested locally, works great. Big thanks for the PR!

Pretty much just a couple nits, would like to hear others ideas for a flag name, but I'm thinking this can definitely be one of the options for the config file. The lesser used flags are perfect for that.
But thanks for including the gen/tests

src/fs/feature/git.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
src/fs/feature/git.rs Outdated Show resolved Hide resolved
MartinFillon
MartinFillon previously approved these changes Apr 23, 2024
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.

LGTM, thanks for updating the tests 😄

@llllvvuu
Copy link
Author

Thanks for the comments! The naming isn't too deep so any suggestions are welcome

Copy link
Member

@PThorpe92 PThorpe92 left a comment

Choose a reason for hiding this comment

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

As we are re-doing the parser anyway here pretty soon, I'm even more alright with the name as worst case it's still temporary.

Thanks again for the PR 👍

@llllvvuu
Copy link
Author

re-doing the parser

Ah right, I thought I'd waited for that, but I'd missed the part where it was merged into a development branch instead of main. It wouldn't be too hard to port to both branches and do a name change. I'd be happy to take that up as well if/when a name is decided.

@MartinFillon
Copy link
Contributor

MartinFillon commented Apr 24, 2024

Ah right, I thought I'd waited for that, but I'd missed the part where it was merged into a development branch instead of main. It wouldn't be too hard to port to both branches and do a name change. I'd be happy to take that up as well if/when a name is decided.

well its on the v1.0 branch rn, if you want you can see the progress on #787
However, if you don't wanna deal with it atm, It will be merged by myself when your pr is merged

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.

Sorry, it seems like there has been significant changes to certain parts of the codebase, creating conflicts with these changes :/

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: --skip-submodules
5 participants