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

Fix #14, use json to store icons #15

Merged
merged 1 commit into from
Oct 22, 2022
Merged

Fix #14, use json to store icons #15

merged 1 commit into from
Oct 22, 2022

Conversation

Freed-Wu
Copy link
Contributor

According to https://github.com/ogham/exa/blob/master/src/output/icons.rs
add some icons

sort all json alphabetically

Because neovim/neovim#20757
Use '.' to replace '' to get default value

And, add icons for some directory, like exa:

Screenshot from 2022-10-21 14-01-50

@Freed-Wu
Copy link
Contributor Author

Error: Command './configure --prefix=/Users/runner/vim-v8.2.0235 --with-features=huge --enable-fail-if-missing' exited non-zero status 1: configure: error: WRONG! uint32_t not defined correctly.

Some test failed. Why?

Copy link
Owner

@lambdalisue lambdalisue left a comment

Choose a reason for hiding this comment

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

Please split and organize your commit logically so that I can review the changes.

I'm sorry I thought multiple things are in one commit because PR describe several things but it seems things are quite small and can be put into a single commit. I'll review the changes.

@lambdalisue
Copy link
Owner

Some test failed. Why?

Never mind. It seems the failure is not caused by this PR but caused by GitHub macOS runner chage. I'll fix it

@lambdalisue lambdalisue self-requested a review October 22, 2022 07:29
autoload/nerdfont/path/pattern.vim Outdated Show resolved Hide resolved
According to https://github.com/ogham/exa/blob/master/src/output/icons.rs
add some icons

Sort all json alphabetically

Remove 'scriptencoding utf-8' because now vim script don't contain icons

Add '\m' to avoid pattern match failure because users change their '&magic'

Because neovim/neovim#20757
Use '.' to replace '' to get default value
Copy link
Owner

@lambdalisue lambdalisue left a comment

Choose a reason for hiding this comment

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

LGTM

@lambdalisue lambdalisue merged commit a82c9b7 into lambdalisue:master Oct 22, 2022
@lambdalisue
Copy link
Owner

Released. https://github.com/lambdalisue/nerdfont.vim/releases/tag/v1.4.0 Thanks for your contribution 🎉

@Freed-Wu
Copy link
Contributor Author

You are welcome.

I still have some advice: Now the json is sorted alphabetically. If you think
sorted is better than unsorted, how about add a test to check if it is sorted
after every PR? If we don't have any test to ensure the sort, after many PRs,
they may be changed to unsorted.

@lambdalisue
Copy link
Owner

I still have some advice: Now the json is sorted alphabetically. If you think
sorted is better than unsorted, how about add a test to check if it is sorted
after every PR? If we don't have any test to ensure the sort, after many PRs,
they may be changed to unsorted.

I also worry about that. Could you add such CI and send me a PR? (I'll do otherwise but it would take time)

@Freed-Wu
Copy link
Contributor Author

Could you add such CI

I'll take a try.

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.

None yet

2 participants