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

Add directory icon #6

Closed
petobens opened this issue Mar 12, 2019 · 10 comments · Fixed by #951
Closed

Add directory icon #6

petobens opened this issue Mar 12, 2019 · 10 comments · Fixed by #951
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@petobens
Copy link

Hi! This is awesome! Thanks for writing it up.

I found one minor thing though which I was wondering if you could change: add custom icon for directories. In the following image you can see fzf (actually fd) running for directories (top pane) and files (bottom pane) with lsd --tree and bat previews respectability. It would be really nice if you could change the directory icon to the one / similar one show by the tree preview:

Screenshot_2019-03-12_17:50:19
@petobens
Copy link
Author

In other words I would expect the following command in my home dir to return directory icons:

fd --type d | devicon-lookup | fzf

but instead I get:
image

@coreyja
Copy link
Owner

coreyja commented Mar 19, 2019

Hey @petobens! Thanks for the kind words! Sorry for taking a bit to get back to you!

This is definitely a feature I would like to add to this project. And I think it should make for a quick feature. I can't promise when I will get to it, but I can promise I would gladly review a pull request 😃! But it is on the list of things I would like to implement eventually

@coreyja coreyja added enhancement New feature or request good first issue Good for newcomers labels Mar 19, 2019
@petobens
Copy link
Author

Awesome. Thanks!

@coreyja
Copy link
Owner

coreyja commented Mar 23, 2019

I've been thinking about this a bit and one issue is that to implement this, this projects scope would have to expand a bit, as it would have to access the filesystem. Right now it solely operates on the stdin, parsing each line as a path and extracting the extension. One nice property of this, is that this utility currently operates the same no matter the directory.

It also feels like a separation of concerns issue, I would like to offload the work of querying the filesystem to another tool, in this case maybe fd. Unfortunately I couldn't find an easy way to determine if the output was a directory simply from the output that was given by fd, but I haven't dived very deep into fd options.

I still think this is a useful addition, since this is a very common use-case, however I want to be thoughtful about how it gets added to this utility!

@coreyja coreyja added needs-fleshed-out Not ready to implement and removed good first issue Good for newcomers labels Mar 24, 2019
@petobens
Copy link
Author

I understand your concerns but I still hope this feature finds its way into the application since, as you've said, it's a very common use case.

coreyja pushed a commit that referenced this issue Mar 31, 2019
@dependabot @coreyja
chore(deps): bump assert_cmd from 0.6.0 to 0.11.1 (#24)
c56431f 
Bumps [assert_cmd](https://github.com/assert-rs/assert_cmd) from 0.6.0 to 0.11.1.
<details>
<summary>Changelog</summary>

*Sourced from [assert_cmd's changelog](https://github.com/assert-rs/assert_cmd/blob/master/CHANGELOG.md).*

> ## 0.11.1 (2019-03-23)
> 
> 
> #### Bug Fixes
> 
> * **stdin:**  Docs didn't work ([2d4756a2](assert-rs/assert_cmd@2d4756a), closes [#71](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/71))
> 
> 
> 
> <a name="0.11.0"></a>
> ## 0.11.0 (2019-01-29)
> 
> 
> #### Performance
> 
> * **cargo:**  Faster bin lookup ([93791474](assert-rs/assert_cmd@9379147), closes [#6](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/6), [#57](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/57), breaks [#](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/))
> 
> #### Breaking Changes
> 
> * **cargo:**  Faster bin lookup ([93791474](assert-rs/assert_cmd@9379147), closes [#6](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/6), [#57](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/57), breaks [#](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/))
>   * As a side-effect, removed `cargo_example` in favor of using `escargot`.
>   * See the [`assert_cmd::cargo` docs](https://docs.rs/assert_cmd/0.11.0/assert_cmd/cargo/index.html) for trade-offs with when to use `escargot` vs `assert_cmd`
> 
> 
> <a name="0.10.2"></a>
> ## 0.10.2 (2018-11-21)
> 
> 
> #### Bug Fixes
> 
> * **assert:**  Support Strings for easy comparison ([81035079](assert-rs/assert_cmd@8103507), closes [#60](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/60))
> * **docs:**
>   * A broken link ([854f7c27](assert-rs/assert_cmd@854f7c2))
>   * List caveats for cargo support.
> 
> 
> 
> <a name="0.10.1"></a>
> ## 0.10.1 (2018-10-10)
> 
> 
> #### Bug Fixes
> 
> * Documentation fixes
> 
> 
> <a name="0.10.0"></a>
> ## 0.10.0 (2018-10-10)
> 
> 
></tr></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`e3070fa`](assert-rs/assert_cmd@e3070fa) chore: Release v0.11.1
- [`7acfee7`](assert-rs/assert_cmd@7acfee7) Merge pull request [#72](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/72) from epage/stdin
- [`2d4756a`](assert-rs/assert_cmd@2d4756a) fix(stdin): Docs didn't work
- [`84f6332`](assert-rs/assert_cmd@84f6332) docs: Update changelog to help with the transition
- [`061a3cd`](assert-rs/assert_cmd@061a3cd) chore: Release 0.11.0
- [`f9b896b`](assert-rs/assert_cmd@f9b896b) Merge pull request [#65](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/65) from assert-rs/dependabot/cargo/escargot-0.4
- [`e5aee5c`](assert-rs/assert_cmd@e5aee5c) Merge pull request [#69](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/69) from epage/cargo
- [`bf5d716`](assert-rs/assert_cmd@bf5d716) chore: Update CI
- [`9379147`](assert-rs/assert_cmd@9379147) perf(cargo): Faster bin lookup
- [`82705d7`](assert-rs/assert_cmd@82705d7) Merge pull request [#66](https://github-redirect.dependabot.com/assert-rs/assert_cmd/issues/66) from AnderEnder/macos-tests-fix
- Additional commits viewable in [compare view](assert-rs/assert_cmd@v0.6.0...v0.11.1)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=assert_cmd&package-manager=cargo&previous-version=0.6.0&new-version=0.11.1)](https://dependabot.com/compatibility-score.html?dependency-name=assert_cmd&package-manager=cargo&previous-version=0.6.0&new-version=0.11.1)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot ignore this [patch|minor|major] version` will close this PR and stop Dependabot creating any more for this minor/major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@coreyja coreyja added good first issue Good for newcomers and removed needs-fleshed-out Not ready to implement labels Jun 5, 2019
@coreyja
Copy link
Owner

coreyja commented Jun 5, 2019 •
edited
Loading

I've come around on this issue recently! I would definitely review PRs that add this functionality, especially ones that do it behind a CLI flag!

Not sure if I will implement this in the near term or not, but it's definitely open if someone wants to take a swing at it!

Edit: To expand I've come around on the thinking that this tool should be more "pure" and only operate on stdin. I think there is more value in adding these types of features than in keeping in the completely single purpose style

@luukvbaal
Copy link

luukvbaal commented Mar 29, 2021 •
edited
Loading

@petobens if you're still interested see #490 (comment).

The script was written with tree -F output in mind which adds a / at the end of directories.
Not sure if thats an option for fd.

@coreyja
Copy link
Owner

coreyja commented Jul 23, 2023

Looking at this issue again today! It's been awhile, but I think I'll try to implement something for this here soon!

My current thoughts are that we will add a new CLI option that turns on this behavior. If the option is NOT set, we will not interact with the filesystem (just like past versions).
However if the options is provided, we will take the value (or assume the CWD if option is value-less) and use that directory as the 'base' for looking up directories.

Each line will be appended to the base path from the CLI. If this path is a directory we will use the folder icon

@petobens
Copy link
Author

Sounds good to me :)

@coreyja
Copy link
Owner

coreyja commented Dec 12, 2023

This will be fixed in #951 when it lands! 🎉 💯

@coreyja coreyja mentioned this issue Dec 12, 2023
Merged
@coreyja coreyja linked a pull request Dec 12, 2023 that will close this issue
Some improvements. #951
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Some improvements. Shuraken007/devicon-lookup
3 participants
@coreyja @petobens @luukvbaal