Skip to content

chore: sort icons alphabetically #543

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

Merged
merged 10 commits into from
Jan 25, 2025
Merged

chore: sort icons alphabetically #543

merged 10 commits into from
Jan 25, 2025

Conversation

gegoune
Copy link
Collaborator

@gegoune gegoune commented Jan 6, 2025

Closes #319

TODO:

  • discuss formatting
  • add CI validation
  • adapt generate_colors.lua
  • refactor generate_colors.lua

What's the consensus for changing format to single line tables like that? This makes sorting alphabetically much easier. We would probably have to find a way to enforce that formatting on CI.

.stylua.toml Outdated
@@ -1,4 +1,4 @@
column_width = 120
column_width = 130
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately doesn't look like stylua has per file overrides.

@gegoune gegoune force-pushed the chore/sort-tables branch from 7726196 to 13f45e3 Compare January 6, 2025 19:52
@gegoune
Copy link
Collaborator Author

gegoune commented Jan 6, 2025

@alex-courtis I am thinking of breaking icon files into separate files holding single table only. Something like:

lua/
	nvim-web-devicons/
 		default_iconset/
			dark/
				by_filename.lua
				by_file_extension.lua
				by_operating_system.lua
				by_desktop_environment.lua
				by_window_manager.lua
			light/
				by_filename.lua
				by_file_extension.lua
				by_operating_system.lua
				by_desktop_environment.lua
				by_window_manager.lua
		other_iconset/
			light/
			dark/

Splitting to individual files would make it easier to sort programmatically.

If we decide to do something like that it best be done in another PR, keeping this one relatively small.

Edit: I just saw you did something similar in #418.

@hasecilu
Copy link
Collaborator

I like the new alignment , I remember Go and Rust formatters do this kind of aligments automatically, was that done with stylua or script?

@gegoune
Copy link
Collaborator Author

gegoune commented Jan 10, 2025

With nvim and mini.align. Shouldn't be too hard to automate it.

@alex-courtis
Copy link
Member

alex-courtis commented Jan 13, 2025

@alex-courtis I am thinking of breaking icon files into separate files holding single table only. Something like:

Edit: I just saw you did something similar in #418.

That sounds fantastic... it's very difficult to see which table you are editing. [{ doesn't really help.

This sounds like the right time, whilst we're touching generation, subsequent PR sounds good.

418 is a proof of concept that needs a lot of work, not sure when I'll get back to it...

@alex-courtis
Copy link
Member

I like the new alignment , I remember Go and Rust formatters do this kind of aligments automatically, was that done with stylua or script?

An alternative is emmylua, which is setup for nvim-tree to format via CLI (also CI): https://github.com/nvim-tree/nvim-tree.lua/blob/7a4ff1a516fe92a5ed6b79d7ce31ea4d8f341a72/Makefile#L19

It has align_continuous_similar_call_args which achieves what we're after.

stylua was retired in favour of emmylua, mainly to allow easier in-editor formatting via vim.lsp.buf.format()
nvim-tree/nvim-tree.lua#2932

@hasecilu
Copy link
Collaborator

That sounds fantastic... it's very difficult to see which table you are editing. ``

And that can make the code confused-contributor proof to prevent distracted maintainers (me) merge PR when key are on wrong table _-_

@alex-courtis
Copy link
Member

This is fantastic, should have done this long ago:

  • far more readable
  • we can finally use sane text processing

@gegoune gegoune marked this pull request as ready for review January 18, 2025 17:52
@gegoune

This comment was marked as outdated.

@hasecilu

This comment was marked as outdated.

@@ -38,7 +38,7 @@ jobs:
with:
token: ${{ secrets.GITHUB_TOKEN }}
version: "0.19"
args: --check lua scripts
args: --respect-ignores --check lua scripts
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

Copy link
Collaborator

@hasecilu hasecilu left a comment

Choose a reason for hiding this comment

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

Made some tests adding new extensions and it's working as expected.


local function iterate_sections()
-- NOTE: technique used (search function and nowrapscan) causes initial search to omit first table
-- Will be simplified in consecutive PR.
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to? Looks good to me...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was referring to future PR which will separate tables into individual files at which point we won't have to iterate over multiple tables in single file any more.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

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

This is just fantastic:

  • so much code deleted
  • generating light completely is super robust - it had so many points of failure
  • we've not had to add any fragile new ci/check/script mechanisms, just extended the existing patterns that we know work

We don't even have to update the doc - this handles everything for the dev and will be checked during CI.

@alex-courtis
Copy link
Member

What sort of commit message could this be?

"icon tables normalised and formatted with one icon per line, light icons completely generated"

@gegoune gegoune merged commit 37334ad into master Jan 25, 2025
5 checks passed
@gegoune gegoune deleted the chore/sort-tables branch January 25, 2025 09:36
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.

chore: sort icons_by_* tables alphabetically
3 participants