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

test-fonts.sh: allow wrapping at any number #1270

Merged

Conversation

hasecilu
Copy link
Contributor

Description

The line decorations are hardcoded to 5 elements, with these changes you just need to change the wrapAt varible and the line decorations length will change as well

Requirements / Checklist

What does this Pull Request (PR) do?

Change the printing method of decorations.

How should this be manually tested?

Just change wrapAt variable and execute the script.

Any background context you can provide?

The default value wastes a lot of usable space in the terminal.

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

Example with length of 16 elements.
Screenshot from 2023-05-30 20-08-06

Sorry, something went wrong.

hasecilu added 4 commits May 30, 2023 20:34
- Each variable was splitted in 3 variables containing the start block, middle block and end block respectively.
- Now the wrapAt variable controls programatically the length of the decorations lines, (for loop).
- Minimal wrapAt allowed value is 3.
- Since the format of the values used are normally hexadecimal seems a reasonably election.
- According to https://www.shellcheck.net/wiki/SC2034 : "For throwaway variables, consider using _ as a dummy".
@hasecilu hasecilu changed the title Test fonts allow wrapping at any number test-fonts.sh: allow wrapping at any number May 31, 2023
@hasecilu
Copy link
Contributor Author

@all-contributors please add @hasecilu for code

@allcontributors
Copy link
Contributor

@hasecilu

I've put up a pull request to add @hasecilu! 🎉

@hasecilu
Copy link
Contributor Author

I think it could be useful to pass the first argument to the wrapAt variable. Something like: ./test-fonts.sh 5

@Finii
Copy link
Collaborator

Finii commented May 31, 2023

Do you want to add that?

Thanks for the PR.

Also maybe we should (re?)enable shellcheck in the CI workflow.

@Finii
Copy link
Collaborator

Finii commented May 31, 2023

Minimal wrapAt allowed value is 3

Maybe that should be checked or at least documented in the script.
Esp if wrapAt becomes a parameter :-}

Copy link
Collaborator

@Finii Finii left a comment

Choose a reason for hiding this comment

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

Very nice!

bin/scripts/test-fonts.sh Outdated Show resolved Hide resolved
bin/scripts/test-fonts.sh Show resolved Hide resolved
bin/scripts/test-fonts.sh Outdated Show resolved Hide resolved
@Finii
Copy link
Collaborator

Finii commented May 31, 2023

Ah, just a note, maybe to myself... Apple has this ancient bash version, one always needs to check on that.

Would really like to get rid of these bash stuff, who uses bash anyhow these days, when most distros already switched to zsh. Also a reason why I usually strive to only use and switch to sh as that at least works everywhere...

hasecilu added 2 commits May 30, 2023 23:45
- Resistant to bad inputs.
- First checks if argument is a number and then checks if its greater than 2, if it's, the `wrapAt` variable is updated.
@hasecilu
Copy link
Contributor Author

Do you want to add that?

Just sent a new commit enabling wrap override.

Also maybe we should (re?)enable shellcheck in the CI workflow.

I think could be fine, from I have seen the scripts are pretty much in good state.

@hasecilu
Copy link
Contributor Author

Maybe that should be checked or at least documented in the script. Esp if wrapAt becomes a parameter :-}

It checks for numbers greater than 2, the user shouldn't be able to break it.

@Finii
Copy link
Collaborator

Finii commented May 31, 2023

Looks very good.
Only things missing

  • Increase script version (minor)
  • Add some example with the parameter in the comment-head

- 1.1.0 version.
- Add example command to change column size.
Copy link
Collaborator

@Finii Finii left a comment

Choose a reason for hiding this comment

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

Thanks!

Finii added 4 commits June 1, 2023 07:37
[why]
The code is hardcoded for 4 (hex-)digit codepoints. If we want to
examine 3 or 5 digit codes it breaks the formatting.

[how]
Switch from \u to \U to allow printing codepoints with more than 4
digits. This works on all machines I tested (Linux/Mac).

Manually pad the codepoint to get a consistent length independent of
actual number of digits. We can not use "%5x" because we want to
underline only the actual digits.
This also makes the output of empty slots nicer because the underline is
skipped if there is no code.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
If a set has multiple ranges defined only the last of that ranges is
actually printed.

[how]
With commit
  7a4b5f8  Fixes build errors (ShellCheck)

a false positive of spellcheck lead to a 'correction' in the code that
actually broke it. The mapfile does not accumulate the sequences but
fills it in and so just the last sequence 'survives'.

Dropping mapfile also enables this script on MacOS, as that ancient
bash does not have mapfile.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
[why]
The "all" range just prints again all the ranges that we previously
already printed (?!!).

As we can not select individual sets this does not make much sense.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii merged commit 05691fc into ryanoasis:master Jun 1, 2023
@Finii Finii mentioned this pull request Jun 1, 2023
2 tasks
@hasecilu hasecilu deleted the test-fonts_allow_wrapping_at_any_number branch July 16, 2023 22:10
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
…ping_at_any_number

test-fonts.sh: allow wrapping at any number
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