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

allow dirs-first flag to work regardless of order #67

Merged
merged 4 commits into from Mar 14, 2023

Conversation

fawni
Copy link
Contributor

@fawni fawni commented Mar 14, 2023

closes #65

@xiuxiu62
Copy link

I checked out your branch and can confirm it works. I'm working on a dirs-last implementation, but thought it might make more sense to roll the two into a single optional sub-command. Something like DirectoryOrdering { First, Last }, with short -D and long --dir-order.

src/render/tree/mod.rs Outdated Show resolved Hide resolved
@fawni
Copy link
Contributor Author

fawni commented Mar 14, 2023

i changed the sort type from Option<SortType> to SortType and set the default value to SortType::None which i think makes the most sense for this implementation. however, this assumes that SortType::None is okay, is it okay? this means that it can be explicitly specified by users: et -s none and i do not know if that is good design or not

@bryceberger
Copy link
Contributor

Yeah, that implementation makes sense. Seems fine to let "sorting by none" be an explicit option, since it's the default. If someone "overrides" the default with an alias (or a config file? not 100% sure how those work with defaults), this gives them a chance to override their default.

That all said, this was an option in 1.3 and was later removed (in 5ea5730). Not sure why / if it was intentional, but there don't seem to be downsides to me.

@solidiquis
Copy link
Owner

@bryceberger I didn't have a great reason to remove it. It just felt weird at the time to have a None variant instead of just using an Option but in light of this discussion I am in agreement.

Thanks @fawni for the contribution! If you wouldn't mind just taking care of the minor merge conflict that'd be awesome.

this seems to have been applied before resolving the conflict
@solidiquis
Copy link
Owner

Thanks @fawni for the contribution! <3

@solidiquis solidiquis merged commit 7de7f5f into solidiquis:master Mar 14, 2023
@fawni fawni deleted the fix/dirs-first branch March 14, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--dirs-first flag does nothing if no SortType is specified
4 participants