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

Second attempt to enable custom tree for 'zinit ls' command #190

Closed
wants to merge 4 commits into from
Closed

Second attempt to enable custom tree for 'zinit ls' command #190

wants to merge 4 commits into from

Conversation

nbrown
Copy link
Contributor

@nbrown nbrown commented Feb 28, 2022

Enable users to specify the zinit ls command via ZINIT[LIST_COMMAND] and
provide default commands for exa and tree. Prefer exa if it is installed.

Addresses issue #170

I cherry-picked the original changes to a clean branch and then updated the docs to attempt to address the documentation action issues.

@nbrown nbrown changed the title Second attempt to enable custom 'zinit ls' command Second attempt to enable custom tree for 'zinit ls' command Feb 28, 2022
@vladdoster
Copy link
Member

vladdoster commented Feb 28, 2022

oof, this zshdocs bug is a doozy

@alichtman
Copy link
Member

Whatever. Let the test fail.

@alichtman alichtman marked this pull request as ready for review February 28, 2022 19:01
@alichtman alichtman self-requested a review February 28, 2022 19:01
Copy link
Member

@alichtman alichtman left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but looks good to me

Copy link
Member

@alichtman alichtman left a comment

Choose a reason for hiding this comment

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

Haven't tested it, but looks good to me

nbrown added 3 commits March 5, 2022 09:05
Enable users to specify the zinit ls command via ZINIT[LIST_COMMAND] and
provide default commands for exa and tree. Prefer exa if it is
installed.
@nbrown
Copy link
Contributor Author

nbrown commented Mar 10, 2022

Well, I managed to make it work by using a docker container used in https://github.com/nektos/act

My conclusion is that there is something different between my macOS en_US.UTF-8 and the one used in the github actions containers.

@nbrown nbrown requested a review from alichtman March 10, 2022 02:53
@alichtman
Copy link
Member

This PR shouldn't contain a massive docs update. That should already be on master. I don't have time to jump into debugging it but I won't merge it with an ~800 line addition diff

@vladdoster
Copy link
Member

@nbrown great job on getting this to pass

giphy

@vladdoster
Copy link
Member

vladdoster commented Mar 10, 2022

@nbrown Since you mentioned this, I did some digging and think I found the root issue.

I went to our Dockerfile and saw we don't explicitly set the locale, which seemed odd. A few GitHub issues later and I found this issue:

dotnet/runtime#962

Your hypothesis seems to be spot on and created an issue to track it.

@nbrown
Copy link
Contributor Author

nbrown commented Mar 10, 2022

This PR shouldn't contain a massive docs update. That should already be on master. I don't have time to jump into debugging it but I won't merge it with an ~800 line addition diff

Sorry for the large doc change, I don't know how you want to handle it but the two choices as I see it are minimal doc changes that don't pass the documentation check or a large change that passes the check. I'll gladly revert it to not pass that check if that's what is preferred.

@alichtman
Copy link
Member

Yeah, let's keep the PR tight and drop the docs change. We'll get the docs test sorted out soon (maybe, idk -- no promises) but I'm fine with it failing here.

This reverts commit e2043bf.
@nbrown
Copy link
Contributor Author

nbrown commented Mar 12, 2022

Yeah, let's keep the PR tight and drop the docs change. We'll get the docs test sorted out soon (maybe, idk -- no promises) but I'm fine with it failing here.

Done. I reverted the doc fix so it's now a minimal change.

On the docs test, I'll leave some info on that bug. I think it primarily comes down to a sorting (collation) inconsistency between Linux and macOS/BSD based on my reading and testing.

Copy link
Member

@vladdoster vladdoster left a comment

Choose a reason for hiding this comment

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

This check evaluates to true even though I have exa, but no tree.

zinit-autoload.zsh Show resolved Hide resolved
@vladdoster vladdoster self-requested a review April 11, 2022 10:05
@vladdoster
Copy link
Member

Closing in favor of #221 due to lack of update on the PR.

@vladdoster vladdoster closed this Apr 25, 2022
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.

3 participants