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

fix: add missing filetypes #531

Merged
merged 10 commits into from
Jan 3, 2025
Merged

fix: add missing filetypes #531

merged 10 commits into from
Jan 3, 2025

Conversation

hasecilu
Copy link
Collaborator

Based on the script: #433 (comment)

@hasecilu hasecilu mentioned this pull request Dec 24, 2024
Merged
@gegoune
Copy link
Collaborator

gegoune commented Dec 24, 2024

Nice! Might be worth adding to required checks on CI.

hasecilu
hasecilu commented Dec 24, 2024
View reviewed changes
scripts/filetypes.sh Outdated
if [ -n "$line" ]; then
continue
else
[ -f "/usr/share/nvim/runtime/syntax/$key.vim" ] &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is valid on Arch Linux but is it the same path on Ubuntu (CI)?

Copy link
Member

Choose a reason for hiding this comment

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

For nvim-tree, we're setting $VIMRUNTIME in the workflow, based on the installed location of rhysd/action-setup-vim@v1
https://github.com/nvim-tree/nvim-tree.lua/blob/5ad87620ec9d1190d15c88171a3f0122bc16b0fe/.github/workflows/ci.yml#L89

The related check script then uses that or defaults to /usr/share/nvim/runtime
https://github.com/nvim-tree/nvim-tree.lua/blob/8f974879a04b93fff68b4627087782e76cdf2ed0/scripts/luals-check.sh#L6

We're using the same action here so everything should Just Work.

hasecilu
hasecilu commented Dec 26, 2024
View reviewed changes
CONTRIBUTING.md
@@ -4,22 +4,24 @@ Thank you for your contribution!

## Order

Please ensure `icons_by_filename`, `icons_by_file_extension` and `filetypes` are ordered alphabetically, to prevent merge conflicts.
Please ensure `icons_by_filename`, `icons_by_file_extension`, `icons_by_operating_system`, `icons_by_desktop_environment`,
Copy link
Collaborator Author

@hasecilu hasecilu Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Line breaks at 120 char but could be 100 or 80 on .md files.

Copy link
Member

Choose a reason for hiding this comment

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

I'm easy either way, although links often end up >80

@alex-courtis
Copy link
Member

Nice! Might be worth adding to required checks on CI.

Yes please!

alex-courtis
alex-courtis requested changes Dec 28, 2024
View reviewed changes
Copy link
Member

@alex-courtis alex-courtis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Fantastic! Working as advertised.

Please:

  • delete scripts/filetype-generator.sh
  • add to CI

scripts/filetypes.sh Outdated
if [ -n "$line" ]; then
continue
else
[ -f "/usr/share/nvim/runtime/syntax/$key.vim" ] &&
Copy link
Member

Choose a reason for hiding this comment

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

For nvim-tree, we're setting $VIMRUNTIME in the workflow, based on the installed location of rhysd/action-setup-vim@v1
https://github.com/nvim-tree/nvim-tree.lua/blob/5ad87620ec9d1190d15c88171a3f0122bc16b0fe/.github/workflows/ci.yml#L89

The related check script then uses that or defaults to /usr/share/nvim/runtime
https://github.com/nvim-tree/nvim-tree.lua/blob/8f974879a04b93fff68b4627087782e76cdf2ed0/scripts/luals-check.sh#L6

We're using the same action here so everything should Just Work.

CONTRIBUTING.md

Add the icon to table **1.** if the icon is for a file that is always named that way, for example `.gitconfig`.
Add the icon to table **2.** if the icon is for all files with an extension, for example `vim`.
Add the icon to table **3.**, **4.**, **5.** if the icon is from an OS, DE or WM.
Copy link
Member

Choose a reason for hiding this comment

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

Thank you!

scripts/filetypes.sh
@@ -0,0 +1,25 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

Shellcheck OK
Looks like we do need bash (not /bin/sh) for the pattern matching.

CONTRIBUTING.md
@@ -4,22 +4,24 @@ Thank you for your contribution!

## Order

Please ensure `icons_by_filename`, `icons_by_file_extension` and `filetypes` are ordered alphabetically, to prevent merge conflicts.
Please ensure `icons_by_filename`, `icons_by_file_extension`, `icons_by_operating_system`, `icons_by_desktop_environment`,
Copy link
Member

Choose a reason for hiding this comment

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

I'm easy either way, although links often end up >80

@alex-courtis
Copy link
Member

Dumb question 1:

Could we generate these mappings at runtime, rather than compile time? Users may have installed other filetypes.

The mapping would still need to exist, for unconventional mappings like ["commit"] = "commit_editmsg",

It looks like we could use vim.fn.getcompletion("", "filetype", false) at startup.

@alex-courtis
Copy link
Member

Dumb question 2:

edit: never mind, I didn't read the doc

@hasecilu
Copy link
Collaborator Author

Could we generate these mappings at runtime, rather than compile time? Users may have installed other filetypes.

Or let users add new filetypes on the config file so if they have more filetypes they know they have to also include it.

@alex-courtis
Copy link
Member

Or let users add new filetypes on the config file so if they have more filetypes they know they have to also include it.

Yes, that is necessary and we have many cases.

I'm happy whichever way you go.

@alex-courtis
Copy link
Member

alex-courtis commented Dec 29, 2024 •
edited
Loading

Looking good, I created a branch to test: #534
It removed asm to test.

The script doesn't fail when a file type is missing:

: ; make filetypes
./scripts/filetypes.sh
Please add "asm" to filetypes Lua table.
: ; echo $?
0
: ; scripts/filetypes.sh
Please add "asm" to filetypes Lua table.
: ; echo $?
0
: ;

It's going to need to return non-zero (1 will do) when a type is missing, so that CI can fail.

@alex-courtis
Copy link
Member

It looks like $VIMRUNTIME is not set in CI; we're going to need to set it as per https://github.com/nvim-tree/nvim-tree.lua/blob/5ad87620ec9d1190d15c88171a3f0122bc16b0fe/.github/workflows/ci.yml#L91

I've instrumented the script to show what's present: https://github.com/nvim-tree/nvim-web-devicons/actions/runs/12539821919/job/34966371519?pr=535

It looks like we can use /home/runner/nvim-stable/share/nvim/runtime

@alex-courtis
Copy link
Member

Beautiful!

https://github.com/nvim-tree/nvim-web-devicons/actions/runs/12590541237/job/35092246843?pr=539

@alex-courtis alex-courtis merged commit 5740b73 into master Jan 3, 2025
5 checks passed
@alex-courtis alex-courtis deleted the fix/filetypes branch January 3, 2025 00:10
@alex-courtis
Copy link
Member

Added filetypes to required CI runs

@hasecilu
Copy link
Collaborator Author

hasecilu commented Jan 3, 2025

Nice! Glad it worked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alex-courtis alex-courtis alex-courtis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hasecilu @gegoune @alex-courtis