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

style: Sort variable name ascending in shell script library #1664

Closed
wants to merge 15 commits into from

Conversation

mdsanima
Copy link
Contributor

@mdsanima mdsanima commented Jun 23, 2024

Optimized the definition of icon variables in i_cod.sh by sorting the name ascending for better responsiveness. This improves code maintainability for better visual representation and allowing for easy localized names for a given icon.

This change only sorts names alphabetically, variable names were not changed, they remained as they were at the beginning.

Description

Please explain the changes you made here.

Requirements / Checklist

  • Read the Contributing Guidelines
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan.
    Issue number where discussion took place: #xxx
  • If this contains a font/glyph add its origin as background info below (e.g. URL)
  • Verified the license of any newly added font, glyph, or glyph set. License is: xxx

What does this Pull Request (PR) do?

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

mdsanima added 3 commits June 23, 2024 08:39
Optimized the definition of icon variables in `i_cod.sh` by sorting
the name ascending for better responsiveness. This improves code
maintainability for better visual representation and allowing for
easy localized names for a given icon.

This change only sorts names alphabetically, variable names were not
changed, they remained as they were at the beginning.
@Finii
Copy link
Collaborator

Finii commented Jun 23, 2024

Thanks for this (still in progress) PR!

I'm not sure I understand the reason for the alphabetic sorting ("more responsiveness")?

At the moment they are sorted by codepoint.
And they are usually autogenerated, so changing the current files does not change anything if the generation is re-triggered.

Usually they are used via the i_all.sh script, that does just set a lot of shell variables (where the order does not matter), and from there the shell is used to access the variables (i.e. glyph names) and values (i.e. codepoints).

These files are not for manual ... interaction?

🤔

Fini

@mdsanima
Copy link
Contributor Author

Oh no :( I sorted the variable names because it's easier for me to find specific icons just by their name and it makes my work much easier if I need a given icon.

@Finii
Copy link
Collaborator

Finii commented Jun 23, 2024

See git log ...

$ git log i_cod.sh
commit 2fa45981ee4b46eded3ceee3e4a5100fee442786
Author: Fini Jastrow <[email protected]>
Date:   Sun Mar 17 14:21:15 2024 +0100

    generate-glyph-info: Fix double entries
    
    [why]
    Two glyphs may not have the same name (normally).
    Our glyphnames.json will break.
    
    [how]
    Do not add two entries with the same name but rather report the
    codepoints in the bottom.
    
    Signed-off-by: Fini Jastrow <[email protected]>

commit 6932ff2d8fe5652301330de76cf260bfd52f2452
Author: Fini Jastrow <[email protected]>
Date:   Sun Mar 17 13:47:24 2024 +0100

    Codicons: Update i_cod.sh
    
    With tool, call:
    
    fontforge generate-glyph-info-from-set.py --font ../../src/glyphs/codicons/codicon.ttf --start ea60 --end ec1e --offset 0 --prefix cod > i_cod.sh_part
    
    Signed-off-by: Fini Jastrow <[email protected]>

it's easier for me to find specific icons just by their name

Maybe you can use the https://github.com/ryanoasis/nerd-fonts/blob/master/glyphnames.json file, which allows very easy access to the names via jq sorted in any order you might need?
And/or use the cheat-sheet?

Just suggestions to solve your (unknown) problem; because I guess what you want to do is not what the i_* files are intended for.

@mdsanima
Copy link
Contributor Author

I've sorted almost all the variables in the lib folder, where the files are located.

If the variables are autogenerated, I can modify the script to generate files sorted as I did. I think this might be useful. For me, alphabetically sorted names are very helpful.

@mdsanima
Copy link
Contributor Author

Maybe you can use the https://github.com/ryanoasis/nerd-fonts/blob/master/glyphnames.json file, which allows very easy access to the names via jq sorted in any order you might need? And/or use the cheat-sheet?

Yes, that's actually what I need. So I guess my sorting is unnecessary. Too bad I didn't find this file earlier.

@mdsanima
Copy link
Contributor Author

But if it was useful, I can change the main file that processes them so that they are sorted.

@Finii
Copy link
Collaborator

Finii commented Jun 23, 2024

I'm not sure if it is useful. They are already sorted right now, but by codepoint instead of name. Which is a more technical aspect of the glyphs, but then, the i_*.sh 'database' is kind of the technical side of keeping all the glyphs organized.

I'm not against sorting it, but the question is ... what is better.

Usually if a new icon is added to some font, it is added in the end and the codepoint-sorted files make it very easy for maintainers to just add them in the bottom of the i_* file. What is also lost in your versions is the number of aliases as they are not very apparent anymore and one would need to search for them explicitly with some not-simple code.

I guess the aliases and keeping them organized is one fact that is against alpha sorting 🤔

mdsanima added 2 commits June 23, 2024 11:23
This update is for added proper name for aliases in Font Awesome
set icons shell script variable names.
@mdsanima mdsanima force-pushed the style/lib-script-sort branch from be260fc to 226b3ae Compare June 23, 2024 11:07
@mdsanima
Copy link
Contributor Author

mdsanima commented Jul 1, 2024

Yes, that's a mistake. The option you suggested @Finii with jq is the best for now.

@mdsanima mdsanima closed this Jul 1, 2024
@mdsanima mdsanima deleted the style/lib-script-sort branch July 1, 2024 17:15
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