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

Allow to choose Nerd fonts versions #2730

Closed
wants to merge 2 commits into from

Conversation

bmichotte
Copy link

I discovered Go this morning, so please feel free to help me to have the code "Go-compliant" :D

  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
    No new tests or update needed (I guess)
  • Text is internationalised (see here)
    N/A
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@stefanhaller
Copy link
Collaborator

Thanks for working on this! You beat me to it, I was also just about to open a PR. Since I was almost done, I decided to publish my PR as well, so we can choose which approach we like better. I decided to patch the icon tables for v2; while it doesn't really feel great to patch tables that are conceptually const, it makes the code that works with the tables much simpler. See #2731.

One thing that's missing from your PR is deprecating the showIcons config and using it only as a fallback when nerdFontsVersion is unset.

@bmichotte
Copy link
Author

Ahah, I love OS for this kind of situation :D

it makes the code that works with the tables much simpler. See #2731.

You're totally right, since I discovered Go this morning, I was quite surprised that there were no ternary operator in Go 🤷 and eventually come to this solution

One thing that's missing from your PR is deprecating the showIcons config and using it only as a fallback when nerdFontsVersion is unset.

I chose to not deprecate it and keep it possible to either choose to display the icons or not as well as choosing version 2 or 3. Both way are valid imo.

@stefanhaller
Copy link
Collaborator

One thing that's missing from your PR is deprecating the showIcons config and using it only as a fallback when nerdFontsVersion is unset.

I chose to not deprecate it and keep it possible to either choose to display the icons or not as well as choosing version 2 or 3. Both way are valid imo.

Jesse explicitly proposed deprecating it here, so I thought it makes more sense to follow that suggestion.

@jesseduffield
Copy link
Owner

I'm going with #2731 over this one because it has the deprecation logic in it and it has fewer touch points, but thanks for making this PR anyway, I appreciate the non-mutative approach. I hope this doesn't dissuade you from contributing to Lazygit in the future!

@bmichotte
Copy link
Author

I hope this doesn't dissuade you from contributing to Lazygit in the future!

@jesseduffield Absolutely not, to be honest, IDC that you have chosen #2731 over mine, I'm just happy that the functionality made it to main 👍

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.

3 participants