search

RRethy / base16-nvim

Neovim plugin for building a sync base16 colorscheme. Includes support for Treesitter and LSP highlight groups.
MIT License
523 stars 77 forks source link

Extend the update tool to also update documentation #103

Open MattSturgeon opened 5 months ago

MattSturgeon commented 5 months ago

Overview

The primary goal of this PR is to make it easier to keep the documentation up do date with the available builtin colorschemes.

This is achieved by writing a themes.txt file while processing the schemes, then later pasting that file into the help-doc and the README.

I opted to include themes.txt in the git repo too, rather than putting it in .gitignore. I figured downstream projects may find it useful to have a plaintext list of available colorschemes.

"extra-schemes"

I dealt with the few schemes missing from upstream by adding an "extra-schemes" directory, which is also scanned when processing schemes. This meant moving the default schemes out of the main lua file. I haven't verified whether this break anything.

I nested this in tools, but TBH I should probably move it to the top-level project directory. extra-schemes is data, not a tool! :joy:

For harmonic colors (#95), I considered implementing an alias system: One yaml file could be indexed to multiple names here. I figured since there's only two instances of aliases currently, it wasn't worth the effort.

Other

I also ended up writing a small nix derivation to make it easier for me to run the script. That can be dropped from the PR if you don't want it.

I think it'd be nice to also have CI scheduled to run the updater periodically, opening a PR if any changes were made. That is out of scope for this PR.

Prefix

I've maintained the status-quo of having the base16- prefix in the README, but not in the help docs. IMO it should not be included in either list, but a note should be added along the lines of:

Colorscheme names should be prefixed with base16- when used with the :colorscheme command (but not the lua setup function). e.g. :colorscheme base16-catppuccin

RRethy commented 5 months ago

Thanks for this! I'll try to review it this week or next week.

I've maintained the status-quo of having the base16- prefix in the README, but not in the help docs. IMO it should not be included in either list

Colorscheme names should be prefixed with base16- when used with the :colorscheme command (but not the lua setup function). e.g. :colorscheme base16-catppuccin

I would rather keep that prefix everywhere. The original reason was because this plugin was just a plugin for me and not meant for larger audience, and I wanted to be able to fuzzy select a colorscheme from this so I was just getting all colorschemes with the prefix base16 ref. If you want to add that blurb I'm fine with that, but no functionality should change.

RRethy commented 5 months ago

I think it'd be nice to also have CI scheduled to run the updater periodically, opening a PR if any changes were made. That is out of scope for this PR.

I'm open to PRs but I probably won't spend time implementing this.

MattSturgeon commented 5 months ago

Thanks for this! I'll try to review it this week or next week.

Thanks for taking a look, looking forward to your feedback :smile:

I would rather keep that prefix everywhere.

The current behavior is that the :colorscheme command requires the prefix, however the require('').setup function does not. In fact, using the prefix with the setup function throws an error.

If you want to add that blurb I'm fine with that, but no functionality should change.

I was only proposing changing the docs, not any functionality. I think it'd be more consistent for the help txt and README.md files to both list the available schemes using the same format. The blurb I proposed was intended to clarify the existing situation, I'll add it either to this PR or #102 when I get chance.

Another option would be to remove the list entirely from the README (not the help txt) and instead link to the themes.txt file being generated.

I think it'd be nice to also have CI scheduled to run the updater periodically, opening a PR if any changes were made. That is out of scope for this PR.

I'm open to PRs but I probably won't spend time implementing this.

:+1:

RRethy commented 5 months ago

I think it'd be more consistent for the help txt and README.md files to both list the available schemes using the same format.

Yea, that was just an oversight on my part.