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

feat: Use perfect hash tables for file types and icons #173

Merged
merged 8 commits into from
Sep 3, 2023

Conversation

cfxegbert
Copy link
Contributor

Stripped down version of pull request ogham/exa#1229. I will add other new icons and add icon constants in other pull requests.

This should match the filetypes and icons on the current eza master branch with the following changes:

  • Icon added for .ninja files. Ninja files were already in the filetype table but did not have an icon.
  • Icon for .dmg changed from zip icon to disk image icon

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

This looks really cool, but since it's such a substantial PR, I won't be able to review it fully as fast as the more trivial PRs.

Currently, it needs to be checked with clippy to pass CI, see https://github.com/eza-community/eza/actions/runs/5996553612/job/16263898274?pr=173

@cfxegbert
Copy link
Contributor Author

This looks really cool, but since it's such a substantial PR, I won't be able to review it fully as fast as the more trivial PRs.

Sorry about the large change. It is mostly pulling various tables in the code into a few tables in the build.rs file.

Currently, it needs to be checked with clippy to pass CI, see https://github.com/eza-community/eza/actions/runs/5996553612/job/16263898274?pr=173

These are compile time generated files. I have suppressed the clippy check unreadable_literal on the generated tables.

@cfxegbert
Copy link
Contributor Author

I just realized I can use the macro version of phf instead of the code generation version. This would allow me to move the tables out of build.rs and back into icons.rs and filetype.rs. This would make it easier for other people looking at adding icons/filetypes to find the tables. Should I go ahead and do this refactor?

@cfxegbert cfxegbert requested a review from cafkafk August 29, 2023 00:28
@cafkafk
Copy link
Member

cafkafk commented Aug 29, 2023

This would make it easier for other people looking at adding icons/filetypes to find the tables.

Then yes, that sounds very good. Icons changes seem very popular.

@cfxegbert
Copy link
Contributor Author

cfxegbert commented Aug 31, 2023

Added constants for most of the commonly used icons.

  • Combined nf-dev-windows (e70f) and nf-fa-windows (f17a)
  • Changed icons for gradle files from nf-dev-java (e256) to nf-seti-gradle (e660)

@cfxegbert
Copy link
Contributor Author

Added constants for the rest of the icons used multiple times.

  • Changed epub icon nf-fae-book_open (e28a) to match ebook and mobi icon nf-fae-book_open_o (e28b)
  • Changed .vscode icon from nf-dev-visualstudio (e70c) to nf-md-microsoft_visual_studio_code (f0a1e)
  • Changed rtf icon nf-md-file_document (f0219) to match rst and txt icon nf-fa-file_text (f15c)

Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

Benchmark 1: eza --blocksize -l --hyperlink --grid --git-repos -x --time-style relative ../ --git --header --icons
  Time (mean ± σ):      1.840 s ±  0.082 s    [User: 3.204 s, System: 2.419 s]
  Range (min … max):    1.765 s …  2.033 s    10 runs
 
Benchmark 2: ./target/release/eza --blocksize -l --hyperlink --grid --git-repos -x --time-style relative ../ --git --header --icons
  Time (mean ± σ):      1.787 s ±  0.031 s    [User: 3.145 s, System: 2.413 s]
  Range (min … max):    1.736 s …  1.828 s    10 runs
 
Summary
  ./target/release/eza --blocksize -l --hyperlink --grid --git-repos -x --time-style relative ../ --git --header --icons ran
    1.03 ± 0.05 times faster than eza --blocksize -l --hyperlink --grid --git-repos -x --time-style relative ../ --git --header --icons

src/info/filetype.rs Show resolved Hide resolved
src/output/icons.rs Show resolved Hide resolved
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

I think this is ready, seems to work as expected

@cafkafk cafkafk force-pushed the perfect-hash branch 2 times, most recently from 5272a24 to a133836 Compare September 2, 2023 16:18
@cafkafk
Copy link
Member

cafkafk commented Sep 2, 2023

I had to interactive rebase to get commit summaries to conform to conventional commits:

FAIL  3a18a6c  wrong type: doc  doc: Add function documentation for get_...
FAIL  23c6432  wrong type: task  task: Add constants for the rest of icon...
FAIL  84239f1  wrong type: task  task: Add constants for most of the comm...

3/10 failed

But in the process, it seems github no longer sees your authorship, even with your set as a coauthor.

Perhaps you wanna do an interactive rebase on your end and the force push it? Just change to commit summaries like I did and it should be fine.

If not this should be ready to merge, just let me know.

@cfxegbert
Copy link
Contributor Author

I have rebased on main and updated the git log messages.

@cafkafk cafkafk merged commit de256b5 into eza-community:main Sep 3, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants