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

Use json to store fontawesome icons #2528

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Use json to store fontawesome icons #2528

merged 1 commit into from
Oct 26, 2022

Conversation

Freed-Wu
Copy link
Contributor

Add a github workflow to check if a json is sorted

Copy link
Owner

@lervag lervag left a comment

Choose a reason for hiding this comment

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

Thanks! I'll have to check this before accepting. You can also check. I would actually not be surprised if this now fails at \faFile*.

Can you explain the github workflow for me? I'm not an expert on that, and I'm curious if we should make it more specific? I.e., now the workflow triggers on all json files, but that does not seem correct if there should be more json files added?

autoload/vimtex/syntax/p/fontawesome5.vim Outdated Show resolved Hide resolved
autoload/vimtex/syntax/p/fontawesome5.vim Outdated Show resolved Hide resolved
@lervag lervag mentioned this pull request Oct 24, 2022
@Freed-Wu
Copy link
Contributor Author

Freed-Wu commented Oct 24, 2022

Can you explain the github workflow for me

          for json in assets/json/*.json ; do
            diff <(jq -S . $json) $json || exit 1
          done

if a json is not sorted, jq -S . will sort it and print the sorted json to
stdout, so diff the stdout and original json will return 1, || exit 1 will
quit the for-loop, Like this:

screenshot

@lervag
Copy link
Owner

lervag commented Oct 24, 2022

Thanks. As I wrote above:

I'll have to check this before accepting. You can also check. I would actually not be surprised if this now fails at \faFile*.

I've checked, and as you see, it now fails:

image

The reason is that the Vim dictionary does not preserve a sorted order, necessarily. So, this means we must either sort the item(DICTIONARY) or store the data as a sorted list in the json file.

Regarding the workflow: I'm still wondering if it is a good idea to trigger this workflow on any json file and to specify that all json files must be sorted? Would it not be more appropriate to restrict it to this specific file?

@Freed-Wu
Copy link
Contributor Author

Would it not be more appropriate to restrict it to this specific file?

Fixed.

store the data as a sorted list in the json file.

Fixed.

Add a github workflow to check if a json is sorted
@lervag
Copy link
Owner

lervag commented Oct 26, 2022

I did some minor profiling, and fascinatingly, doing the json_decode(fileread()) is faster than inlining in vimscript.

I'm merging this now. Thanks for contributing!

lervag added a commit that referenced this pull request Oct 26, 2022
@lervag lervag merged commit 0730c35 into lervag:master Oct 26, 2022
@lervag
Copy link
Owner

lervag commented Oct 26, 2022

I made a few minor changes to the workflow (https://github.com/lervag/vimtex/blob/master/.github/workflows/assets.yml), mostly that I fixed the path to the fontawesome file. That will avoid any surprises if I were to add more json files to the assets directory. I believe I should probably collect all assets within that subdirectory, so I might look into that in the not so far away future.

@lervag
Copy link
Owner

lervag commented Oct 26, 2022

Hmm. For some reason, the updated workflow does not run properly now:

https://github.com/lervag/vimtex/actions/runs/3332974138

Perhaps you could assist me here? Here's the diff of the workflow file from your version to the current:

image

@Freed-Wu
Copy link
Contributor Author

Fixed 👍

@lervag
Copy link
Owner

lervag commented Oct 27, 2022

Thanks! I'll look into the new PRs later today!

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.

2 participants