-
-
Notifications
You must be signed in to change notification settings - Fork 170
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(fs)!: add --only-link option and changed --only-file and --only-d… #518
feat(fs)!: add --only-link option and changed --only-file and --only-d… #518
Conversation
f6fcecf
to
4d9f1f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need the PR summaries to be "!: " (notice the !
for breaking change).
my bad |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good code wise, just needs one nit and completions for zsh, bash, fish, and nushell
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add the !
to the commit summary of the breaking change as well
f3fb4a0
to
3a4bf13
Compare
d9c76ec
to
eb7fa12
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm doing something wrong, but this says the arguments doesn't exist (eza: Unknown argument --only-links
)? Maybe the git history got messed up 🤷♀️ ? Also this probably needs completions.
That is clearly not supposed to happen gonna investigate |
4f58415
to
3a054e6
Compare
Ok in fact I was dumb, but that is also due to the parser being so great |
3a054e6
to
db69a60
Compare
Okay I can't seem to find the way to run the command |
8c0671c
to
ffe1335
Compare
You can't currently do that, but we have |
fixed the last one lasting |
a6b3b07
to
69a3f09
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, briefly tested and it works 👍
8a1a9e9
to
354a611
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked on latest version, all work except:
cargo run -- -l --only-dirs --only-links
cargo run -- -l --only-files --only-links
They also show dirs and files that aren't links
😅 lol good catch I didn't test them together |
e888ae6
to
524dd4a
Compare
524dd4a
to
ba883c6
Compare
044d52e
to
2c35ba4
Compare
3e42946
to
2bafa41
Compare
made some changes @cafkafk if you wanna see |
2bafa41
to
0d93a53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be some conflicts, after fix I'll take a look 👍
Gonna fix all of that when I have time |
0d93a53
to
43e370a
Compare
cdee0d9
to
3fc87d6
Compare
…dir behaviour --only-link now only shows links Adeed --only-links option Updated tests BREAKING CHANGE: --only-file now only shows files and links to files --only-dir now only shows directories and links to directories
3fc87d6
to
f410e59
Compare
Superseeded by #1028 |
…ir behaviour
--only-link now only shows links
BREAKING CHANGE:
--only-file now only shows files and links to files --only-dir now only shows directories and links to directories
closes #517
closes #516