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

Extend the update tool to also update documentation #103

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

MattSturgeon
Copy link

@MattSturgeon MattSturgeon commented Jun 13, 2024

  • tools: add default.nix
  • tools: update docs automatically
  • tools: add extra-schemes for themes missing from base16-schemes
  • tools: sort schemes by name
  • Add harmonic to extra-schemes
  • Move schemer to extra-schemes
  • Update schemes

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! 😂

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

Allow building/running the updater using [nix].

[nix]: https://nixos.org
Not all builtin colorschemes come from base16-schemes. Rather than
having various workarounds, we can just define extra schemes in the
repo.
Previously schemes were sorted by _filename_. The `.yaml` file extension
caused the sorting order to be incorrect for some schemes.

Solved by first indexing scheme names in a first-pass loop, then looping
over those (sorted) names to actually process the schemes.
Seems it is missing from upstream.
IDK if there was a technical reason why schemer was defined directly
within `lua/base16-colorscheme.lua`, but now that "extra-schemes"
exists, it can be moved there.

This means the scheme doesn't need a special case for the updater not to
break its vim wrappers, `colors/base16-harmonic-*.vim`
@RRethy
Copy link
Owner

RRethy commented Jun 17, 2024

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
Copy link
Owner

RRethy commented Jun 17, 2024

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
Copy link
Author

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

Thanks for taking a look, looking forward to your feedback 😄

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.

👍

@RRethy
Copy link
Owner

RRethy commented Jun 19, 2024

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.

@bezhermoso
Copy link
Contributor

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.

I have a proposal (#105) that minimize the work to automate a lot of this.

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