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.

Sign up for GitHub

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

Jump to bottom

(exa PR) 1064: Add -A option for more compatibility with ls #43

Merged
merged 11 commits into from
Jul 30, 2023
Merged

(exa PR) 1064: Add -A option for more compatibility with ls #43

merged 11 commits into from
Jul 30, 2023

Conversation

cafkafk
Copy link
Member

@cafkafk cafkafk commented Jul 29, 2023 •
edited
Loading

@cafkafk cafkafk changed the title (exa PR) 1064 (exa PR) 1064: Add -A option for more compatibility with ls Jul 29, 2023
@sbatial sbatial mentioned this pull request Jul 29, 2023
Closed
@sbatial sbatial self-assigned this Jul 29, 2023
sbatial added 8 commits July 29, 2023 21:24
@sbatial
Copy link
Collaborator

sbatial commented Jul 30, 2023

I arrived at an implementation that does what we want and is pretty concise imo

One thing I'd like your opinion on: I think it makes sense to not err when --tree --all --all is used but to fall back to --tree --all.
I find it a bit pedantic for a tool to be very much aware of the problem but not to try and react to it
(I'd do that in a different PR though. does not fall in the scope of this one)

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

I think it makes sense to not err when --tree --all --all is used but to fall back to --tree --all.

I agree!

I find it a bit pedantic for a tool to be very much aware of the problem but not to try and react to it

Wym by this?

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

Your changes seem to work without breaking anything, and cargo test works. Testing the -A flag also seems to work for me.

Is this ready to merge?

@cafkafk cafkafk linked an issue Jul 30, 2023 that may be closed by this pull request
About the -A flag #70
Closed
@sbatial
Copy link
Collaborator

sbatial commented Jul 30, 2023

I find it a bit pedantic for a tool to be very much aware of the problem but not to try and react to it

Wym by this?

Well when --tree --all --all is used right now an error is thrown that that is not possible.
But we know why it is not possible (as far as I'm aware): it would lead to infinite recursion.
And we also know a (imo) solution that is reasonable to assume that that is what the user tried to do.

@sbatial sbatial merged commit 02189ea into main Jul 30, 2023
@sbatial sbatial deleted the pr-1064 branch July 30, 2023 12:03
@sbatial sbatial mentioned this pull request Jul 30, 2023
Closed
63 tasks
This was referenced Sep 28, 2023
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sbatial sbatial sbatial approved these changes

Assignees

@sbatial sbatial

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

About the -A flag
3 participants
@cafkafk @sbatial @kejadlen